Repository navigation
require https authorization endpoint in cupsOAuthMakeAuthorizationURL - #193
Open
tanjiroK-coder wants to merge 1 commit into
Open
tanjiroK-coder wants to merge 1 commit into
tanjiroK-coder wants to merge 1 commit into
Conversation
michaelrsweet
requested changes
Oct 23, 2026
michaelrsweet
left a comment
michaelrsweet
left a comment
Member
There was a problem hiding this comment.
Some minor feedback; you can probably just add the https check to line 1651...
| - Fixed a memory leak in `httpGetSecurity` when using GNU TLS. | ||
| - Fixed a memory leak in `cupsJWTImportString`. | ||
| - Fixed a bug in `cupsJWTExportString` with JSON output. | ||
| - Fixed `cupsOAuthMakeAuthorizationURL` to only accept HTTPS authorization |
Member
There was a problem hiding this comment.
Remember to just put the changelog in the description. We'll add it after merging with a reference to the PR...
| // allow HTTPS URLs (RFC 6749 section 3.1)... | ||
| if (strncmp(authorization_ep, "https://", 8)) | ||
| { | ||
| _cupsSetError(IPP_STATUS_ERROR_INTERNAL, _("Authorization endpoint is not an HTTPS URL."), true); |
Member
There was a problem hiding this comment.
Since we aren't setting the last error for the initial range checking, I don't think we want to do so here either.
This branch has not been deployed
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
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.
cupsOAuthGetAuthorizationCode opens the URL from cupsOAuthMakeAuthorizationURL with LSOpenCFURLRef, xdg-open or start, and that URL is built from the authorization_endpoint value in the server's metadata without any check on its scheme. The metadata comes from whatever server auth_uri names, which for IPP OAuth is the printer's oauth-authorization-server-uri, so a malicious printer or authorization server can return file:///Applications/Calculator.app, smb://host/share/x.app or any registered custom scheme and libcups will hand it to the desktop's URL handler instead of a browser. A plain http:// endpoint is also accepted, which sends the client_id, state and PKCE challenge in cleartext. I confirmed it with a small program linked against libcups3.a: those four endpoints all came back as launchable URLs before the change and are rejected after it, while https endpoints produce the same URL as before. RFC 6749 section 3.1 requires TLS for the authorization endpoint, so the check rejects anything that isn't https:// and lives in cupsOAuthMakeAuthorizationURL, which covers cupsOAuthGetAuthorizationCode and any application that opens the returned URL itself. testoauth needs a live authorization server and isn't part of
make test, so I didn't add a test there; the cups unit tests still pass.