Fix percent-decoding for custom OAuth provider identifiers issues #2541 #2554
Open
big1231945 wants to merge 2 commits into
Open
Fix percent-decoding for custom OAuth provider identifiers issues #2541 #2554big1231945 wants to merge 2 commits into
big1231945 wants to merge 2 commits into
Conversation
… endpoints
The admin GET/PUT/DELETE custom OAuth provider endpoints read the provider
identifier from the URL path via chi.URLParam and required it to start with
the "custom:" prefix. chi routes on (and returns) the raw, still
percent-encoded path segment, so when a browser encodes the ':' as "%3A"
(encodeURIComponent("custom:line") == "custom%3Aline") the handler saw
"custom%3Aline", failed the strings.HasPrefix(..., "custom:") check, and
returned:
identifier must start with 'custom:' prefix, e.g. 'custom:custom%3Aline'
This made it impossible to fetch, update, or delete a custom provider from
the dashboard, even though creation (which reads the identifier from the
JSON body) worked fine. The existing tests masked the bug because they
interpolated the identifier into the path with a literal ':' (leaving
URL.RawPath empty, so chi returned the decoded value).
Add providerIdentifierFromPath, which percent-decodes the path parameter
with url.PathUnescape before the prefix check and DB lookup, and use it in
all three handlers. Cover the fix with a DB-free routing test plus
encoded-identifier GET/PUT/DELETE integration tests.
https://claude.ai/code/session_013p2FfvkpPJwAax6aoj8r57
fix(api): decode percent-encoded identifier for custom OAuth provider endpoints
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What kind of change does this PR introduce?
Bug fix.
What is the current behavior?
#2541
Managing a custom OAuth/OIDC provider from the dashboard fails. Fetching, updating, or deleting a provider returns:
Failed to delete custom OAuth provider: identifier must start with 'custom:' prefix, e.g. 'custom:custom%3Aline'
Root cause: the admin GET / PUT / DELETE handlers in internal/api/custom_oauth_admin.go read the provider identifier from the URL path with chi.URLParam and require the custom: prefix. chi routes on (and returns) the raw, still percent-encoded path segment. Browsers encode the : in custom:line as %3A (encodeURIComponent("custom:line") === "custom%3Aline"), so the handler received custom%3Aline, failed the strings.HasPrefix(identifier, "custom:") check, and returned the error above. The %s in the example message (custom:custom%3Aline) is the raw value the server received — confirming the colon arrived encoded and was never decoded.
Creation was unaffected because it reads the identifier from the JSON body, which is not URL-encoded.
Existing tests missed it because they built the path with a literal colon (/admin/custom-providers/custom:foo). With a raw colon Go leaves URL.RawPath empty and chi returns the decoded URL.Path; real browsers send %3A, which populates RawPath and makes chi return the encoded segment.
What is the new behavior?
The path identifier is percent-decoded before the prefix check and the database lookup, so the dashboard can fetch, update, and delete custom providers again. Raw-colon paths keep working and missing-prefix identifiers are still rejected.
Added providerIdentifierFromPath, which url.PathUnescapes the chi.URLParam value before validation, and used it in all three handlers (removing the duplicated inline validation).
Other path parameters in the codebase are UUIDs, which contain no characters a browser would percent-encode, so they are unaffected.
Tests:
TestProviderIdentifierFromPathDecoding — DB-free test driving the real chi routing: asserts custom%3Aline → custom:line, that a raw colon still works, and that a missing prefix is still rejected.
TestGetProviderWithEncodedIdentifier, TestUpdateProviderWithEncodedIdentifier, TestDeleteProviderWithEncodedIdentifier — full-handler integration tests using a custom%3A... path, reproducing the exact dashboard scenario.
Additional context
go build, go vet, and gofmt -l are clean on the touched files. The DB-free routing test passes locally; the DB-backed integration tests run in CI (Postgres was not available in the authoring environment).