Skip to content

feat(sub): warn when salamander settings cannot reach the client - #6177

Open
n0ctal wants to merge 1 commit into
MHSanaei:mainfrom
n0ctal:upstream-warn-unexpressible-salamander
Open

feat(sub): warn when salamander settings cannot reach the client#6177
n0ctal wants to merge 1 commit into
MHSanaei:mainfrom
n0ctal:upstream-warn-unexpressible-salamander

Conversation

@n0ctal

@n0ctal n0ctal commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Summary

A hysteria2 URI can express salamander obfuscation only as obfs=salamander + obfs-password. When the inbound's finalmask carries more than a password, the emitted link silently describes a server no standard client can reach. Log the keys that cannot be carried.

Why

The comment above the current code is right that emitting the non-standard fm=<json> dump breaks mihomo and other clients, so only the standard fields are written. The gap is what happens when the standard fields are not sufficient to describe the server.

packetSize is the case I hit. With it set, a client built from the generated link applies plain salamander, the server expects the padded variant, and the handshake fails — no error on the client, nothing in the panel, and traffic accounting stays at zero because no session is ever established. It reads as "hysteria2 is broken" rather than "this inbound cannot be expressed as a link", and it survived weeks of looking in the wrong place before a controlled A/B on a live server isolated it: same client, same credentials, same port — connects when the server has only password, fails when packetSize is present.

This does not change the link, so nothing that works today stops working. It makes the reason discoverable.

Scope

  • internal/sub/service.goextraSalamanderKeys lists salamander settings beyond password; the hysteria2 link builder logs a warning naming the inbound and those keys.
  • internal/sub/salamander_uri_test.go — expressible settings report nothing, packetSize is reported.

Validation

  • go test ./internal/sub/ -count=1 green; go build ./..., go vet clean, gofmt clean.
  • Behaviour confirmed on a live deployment: with packetSize present, Happ and INCY both fail on every hysteria2 entry and the inbound records no traffic; removing it makes the same links work immediately.

Risk

None to the data plane. Warning-only, emitted at link build time, no change to the URI or to any stored setting.

@n0ctal
n0ctal marked this pull request as draft August 5, 2026 01:20
@n0ctal

n0ctal commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Why the checks are red — none of it is from this PR.

  • frontendnpm audit --omit=dev --audit-level=high reports a high-severity advisory in the transitive brace-expansion. The same job fails on main itself: run 30898275209 on 38838827, the commit this PR branches from.
  • handle-pr-review — the review workflow ends with The review run ended without commenting, i.e. the reviewer never posted. Nothing to do with the diff.
  • golangci — every remaining finding is SA5011 in files this PR does not touch: internal/xray/api_shadowsocks_test.go, internal/xray/api_users_e2e_test.go, internal/xray/api_wireguard_test.go.

golangci passes on main at the same commit with the same linter (v2.12.2), so this looks like an analysis-cache difference between the push and pull_request runs rather than a real regression — worth a look on your side, but it is not something this branch introduced.

Local gate on the branch: go build ./..., go vet ./..., gofmt -l and the touched package's tests are all clean.

@n0ctal
n0ctal marked this pull request as ready for review August 5, 2026 09:19
@n0ctal

n0ctal commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up on the two remaining red checks, with what I could verify from here.

golangci — the tree is clean; this looks like a stale analysis cache, not a real finding.

I checked out upstream/main unmodified, installed the same linter CI resolves (golangci-lint v2.12.2, version: latest picks it in both runs) and ran it:

  • golangci-lint run over the whole module → 0 issues
  • golangci-lint run ./internal/web/service/..., the package CI reports SA5011 in → 0 issues

The same job is green on main at the very commit these PRs branch from, and the reported files are ones no PR here touches. The findings themselves are the t.Fatal-then-dereference shape, which staticcheck does not flag on a cold run. golangci-lint-action restores an analysis cache (Cache hit for: golangci-lint.cache-Linux-2952-…), so busting that cache is the thing I would try first — I cannot do it from a fork.

frontend — that one is real and fixable: npm audit --omit=dev --audit-level=high trips on GHSA-rgw5-rvv9-x895 via swagger-ui-react → … → brace-expansion@5.0.8. It fails on main too. Opened #6180 with a scoped override to the patched 5.0.9; merging it turns this job green for every open PR, including this one.

Nothing in either check comes from this branch.

@n0ctal
n0ctal force-pushed the upstream-warn-unexpressible-salamander branch from 3353fad to 3645212 Compare August 6, 2026 20:56
A hysteria2 share link carries obfuscation as obfs=salamander plus
obfs-password, and nothing else. Xray's finalmask accepts more than that —
packetSize among them — and those extra settings change what the server expects
on the wire. The emitted URI then looks complete but describes a server the
client cannot reach: every standard client applies plain salamander, the server
drops the packets, and the failure is silent on both ends.

Log the unexpressible keys when building such a link, naming the inbound, so the
cause is visible instead of appearing as a client-side problem.
@n0ctal
n0ctal force-pushed the upstream-warn-unexpressible-salamander branch from 3645212 to 11c46f5 Compare August 8, 2026 02:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant