Repository navigation
Conversation
Coverage Report for CI Build 42878917926Coverage remained the same at 87.062%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
umputun
left a comment
umputun
left a comment
There was a problem hiding this comment.
the section calls itself "a safe flow", but only the email path is safe as written. Two things need to change before merge.
only email has an identity the app can bind. README.md:551 names the Telegram token as an attack path and README.md:566 says the confirmation screen must show something an attacker cannot fake, but the only recipe is for email. A Telegram user is a hash of the chat id plus a first_name and an avatar the attacker can copy from the victim. A user in the middle of linking who opens the attacker's confirmed /auth/telegram/login?token=... link sees "move your anonymous data to ?" and confirms. Oauth and the password direct provider have the same gap. Pls say that only an email login can be bound at step 2, and that for other providers the move rests on the user recognizing the account, or that linking should not be offered across them. The comments at v2/linking_example_test.go:324 and :340 present the path without an address as covering Telegram and oauth, they need the same correction.
the example never binds the destination provider. In confirm (v2/linking_example_test.go:106) the address check is skipped when p.address is empty, so a link started for Telegram or oauth accepts any later login as the destination, including an attacker's email login. Its screen shows an opaque email_<hash> and a name the attacker picked. A start without an address, then the attacker's email confirmation link, then a confirm moves the drafts to the attacker. The design in #250 kept "what the user is about to log in with" in the pending record, step 2 narrowed it to the email address. The smaller fix is to limit the example and step 2 to email with a bound address. The alternative is to store the provider at start, reject a destination from another provider, require the address for email, and add a test for an address-free start followed by an email login.
minor
README.md:568: the query param and header names are configurable (Opts.JWTQuery,Opts.JWTHeaderKey). Pls name the options with their defaultstokenandX-JWT, otherwise an app with a customJWTQuerycopies a check that matches nothingREADME.md:564: step 3 should say confirm refuses a user who is still anonymous. The example has it (v2/linking_example_test.go:90) and the README doesn't. With the example's append-then-delete move, a confirm by the same anonymous user wipes his data
nits, non-blocking
cookieTokenOnlyis called only fromlinkAppmethods, make it a method- the scenario comments inside test bodies (
:309,:324,:340-342) can go into the test names or therequiremessages
Apps that let users start anonymously want to move that data to the account the user logs in with later. The library keeps no users, so this is app code, and the obvious version, merging right after login, hands the data to an attacker whose confirmation link the user opened. The README now describes a flow with an explicit confirmation step, and an example test runs it against the real handlers.
A link started without an email address accepted any later login as the destination, so an attacker's email confirmation link could receive the data of a user who meant to link a Telegram or oauth account. The pending record now keeps the provider, confirm refuses users of other providers, and an email link requires the address. The README says that only email binds a known identity and names the configurable JWT query and header.
a7d2384 to
8b2cc0e
Compare
|
Addressed in 8b2cc0e, with the second option. I also rebased the branch onto master after #326 and #327 merged; the first commit only moved.
|
umputun
left a comment
umputun
left a comment
There was a problem hiding this comment.
both blockers are resolved, the provider binding and the test for a start without an address are what I asked for. Two small things left.
README.md:565: "user ids start with<provider>_" holds for the built-in oauth, Apple, Telegram, direct and verify providers. The dev provider,NewCustomServerwith its default mapper, a customMapUserFnandAddCustomHandlercan return ids without that prefix, so an app linking through one of them gets 403 on every confirm. Pls say the example relies on ids prefixed with the provider name, and that custom and dev providers need their own way to identify the providerv2/linking_example_test.go:101: the nonce cookie takesSecurefromr.TLS, which is always nil behind a TLS-terminating proxy, while the JWT cookie followsOpts.SecureCookies. For a link without an address the nonce alone is enough to move the data: whoever reads it during the 10 minutes logs into his own account on that provider and confirms. Pls takeSecurefromOpts.SecureCookiesthe same way the example takes the JWT names, addSecureto the cookie attributes in step 2, and check it inTestLinkAnonymousUser_NonceCookieAttributes
nit: the trailing comment at v2/linking_example_test.go:524 can go into the first assert.False message, like the other scenario comments
Behind a TLS-terminating proxy r.TLS is nil, so the nonce cookie went out without Secure even when Opts.SecureCookies secured the JWT cookie, and for a link without an address the nonce alone is enough to move the data. The example now takes Secure from Opts.SecureCookies, like the JWT cookie, and the attributes test checks it for both settings. The README says the example tells the provider by the id prefix, which the dev provider and custom providers may not set.
|
Done in 3236b21:
|
umputun
left a comment
umputun
left a comment
There was a problem hiding this comment.
round-2 items check out. One new problem in the email path, plus two smaller ones.
the link expires before the email letter does. v2/linking_example_test.go:104 keeps the pending record and the nonce cookie for 10 minutes, while the confirmation token is valid for 30 (v2/provider/verify.go:364). A user who opens the letter at minute 15 gets logged into the email account, his anonymous JWT is replaced, and confirm returns 409. He can't start again because he is no longer anonymous, and a new anonymous login gets a new random id, so the drafts stay under an id nobody can reach. TestLinkAnonymousUser_ExpiredLink runs exactly this sequence and checks the data is still under anonID. This is the email path the README recommends as the safe one.
the record has to outlive the login it waits for. For a link with an address the nonce alone moves nothing, so it can live as long as the token, 30 minutes plus time to confirm, since the token is issued after start. Links without an address can keep the short window. Pls take expires and MaxAge from one value instead of 10 * time.Minute and 600, add a test where the confirmation comes after minute 10 and the data still moves, and say in step 2 that the record must outlive the login.
minor
v2/linking_example_test.go:93and:79:"email"is hardcoded, while the verify provider builds ids from the name it was registered with (v2/provider/verify.go:285). An app that registersAddVerifProvider("mail", ...)and starts a link formailwithout an address passes the check at:93, andacceptsfalls back to the prefix check, so the address binding is gone and the attacker's ownmail_letter passes. Pls keep the provider name in one const shared withAddVerifProvider, and write the step 3 formula as"<verify provider name>_" + token.HashID(...). Same reason as theJWTQuerypoint in round 1README.md:564: the nonce cookie is fixedSameSite=Lax. An app framed by another site (SameSiteCookie: http.SameSiteNoneModewithPartitionedCookies) never gets the nonce back on confirm, so every confirm returns 409 after start already succeeded. One sentence in step 2 is enough: a framed app sets and clears the nonce cookie withOpts.SameSiteCookieandOpts.PartitionedCookieslike the JWT cookie. It covers only the in-frame logins (direct, telegram, email), not oauth popups, and the XSRF header check stays on
test nits
v2/linking_example_test.go:543: the comment can go into theSecureassert message, like the other onesTestLinkAnonymousUser_AccountSwitchAfterConfirmScreen: add amove_to=alicePOST right after the login as mallory, before the reload, expecting 409. Without it a guard that comparesmove_toonly withp.shownTopasses the whole suite and lets the old screen move the data to mallory
Previously, the README did not say how to move data created by an anonymous user to the account they log in with later, and the obvious approach, merging right after login, is unsafe: the email confirmation link and the Telegram token work from any browser. After this change, a README section describes the flow from discussion #250: unguessable anonymous ids, a pending link started by the anonymous user, and a move only after the user confirms the account shown, bound to the typed address for email logins.
v2/linking_example_test.goimplements the app side of that flow against the real handlers, with the anonymous direct provider and the email provider. It covers the full link, the attack where the user opens the attacker's confirmation link, an old confirmation screen after the browser was logged into another account, a confirm by a user who is still anonymous, an expired link, a replayed nonce, and a token passed in the query or the JWT header. Removing any one of the checks makes a test fail. No library code changes.