Skip to content

backend: require HTTPS AOPP callbacks - #4342

Open
benma-agent wants to merge 1 commit into
BitBoxSwiss:masterfrom
benma-agent:benma-agent/restrict-aopp-callback-https
Open

backend: require HTTPS AOPP callbacks#4342
benma-agent wants to merge 1 commit into
BitBoxSwiss:masterfrom
benma-agent:benma-agent/restrict-aopp-callback-https

Conversation

@benma-agent

@benma-agent benma-agent commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Reject AOPP requests whose callback is not an absolute HTTPS URL
with a non-empty host. Allow redirects only while HTTPS is preserved
so signed addresses and extended public keys cannot be downgraded to
cleartext transport.

Make AOPP error rendering tolerate missing or malformed callback URLs.
Cover callback validation, HTTPS redirects, HTTP downgrade rejection,
and missing callbacks in backend and frontend tests.

@benma
benma requested a review from bznein August 20, 2026 15:07
@benma
benma marked this pull request as ready for review August 20, 2026 15:08
@benma
benma requested a review from a team as a code owner August 20, 2026 15:08
@benma-agent
benma-agent force-pushed the benma-agent/restrict-aopp-callback-https branch from 9a77438 to 25f81c3 Compare August 20, 2026 15:08

@bznein bznein left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex findings:

• The HTTPS restriction can still downgrade through redirects, and its newly rejected
  HTTP path sends frontend state that crashes error rendering. Both defects undermine
  the intended secure failure behavior.

  Full review comments:

  - [P1] Preserve rejected callbacks for error rendering — /home/bznein-coding-agent/
    agent-work/bitbox-wallet-app/backend/aopp.go:260-262
    When an AOPP URI contains a parseable HTTP callback, this branch sets the error
    before backend.aopp.Callback is assigned, so the frontend receives an empty
    callback. The error view calls new URL(aopp.callback).host in frontends/web/src/
    components/aopp/aopp.tsx, which throws and blanks the flow instead of displaying
    aoppInvalidRequest; preserve the parseable callback or make error rendering tolerate    absent callbacks.

  - [P1] Reject cleartext redirect targets — /home/bznein-coding-agent/agent-work/
    bitbox-wallet-app/backend/aopp.go:257-259
    When an accepted HTTPS callback responds with a 307 or 308 redirect to an HTTP URL,
    Go's default http.Client.Post follows it and resends the signed address and optional    xpub over cleartext. Validating only the initial URL therefore does not enforce the
    new transport guarantee; disable redirects for this callback or reject every
    redirect whose target is not HTTPS.

Comment thread backend/aopp.go Outdated
backend.aoppSetError(errAOPPInvalidRequest)
return
}
if !callbackURL.IsAbs() ||

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] I think we can avoid this check, because this simply does:

func (u *URL) IsAbs() bool {
	return u.Scheme != ""
}

And if the scheme is empty it would fail anyway the EqualFold check blow.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Reject AOPP requests whose callback is not an absolute HTTPS URL
with a non-empty host. Allow redirects only while HTTPS is preserved
so signed addresses and extended public keys cannot be downgraded to
cleartext transport.

Make AOPP error rendering tolerate missing or malformed callback URLs.
Cover callback validation, HTTPS redirects, HTTP downgrade rejection,
and missing callbacks in backend and frontend tests.
@benma-agent
benma-agent force-pushed the benma-agent/restrict-aopp-callback-https branch from 25f81c3 to 351a1b9 Compare August 20, 2026 20:59
@benma
benma requested a review from bznein August 20, 2026 21:03
@benma

benma commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Addressed. It's overengineered for sure for something that is basically a non-issue in practice, but oh well...

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.

3 participants