docs(auth, avatar): fix misleading and stale docstrings around the security fix#291
Merged
Conversation
Coverage Report for CI Build 26202851038Coverage increased (+0.03%) to 85.425%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
paskal
force-pushed
the
fix/csp-consumer-note
branch
from
ca93d66 to
0da9c4f
Compare
paskal
changed the title
paskal
added a commit
that referenced
this pull request
May 21, 2026
Pre-existing comment drift in files unrelated to the recent security fix — unrelated docstrings that misled, said the wrong thing, or were copy-paste leftovers. Mirrored across v1 and v2. The security-fix docstring fixes (avatar/avatar.go security comments and auth.go withSecurityHeaders) live in PR #291 and are intentionally NOT included here so the two PRs stay independent. Findings: * avatar/gridfs.go — ID doc claimed MD5 sourced from gridfs; metadata.hash is the sha1 written at Put time. * avatar/bolt.go, avatar/gridfs.go — Put docs and BoltDB type doc claimed these layers "resize" the image; they only copy bytes. Resize happens in Proxy.resize upstream. * avatar/store.go — Migrate doc didn't mention that per-avatar Get/Put/Close errors are logged and skipped, and that the returned count is "ids attempted", not "ids stored". * provider/oauth1.go — initOauth1Handler doc and DEBUG log both said "oauth2". * provider/service.go — Service.Handler doc said "returns auth routes"; it dispatches login/callback/logout. * provider/telegram.go — processUpdates claimed to return an offset (no return value); checkToken doc described an "address or empty string" return shape but the signature is (*token.User, error). * provider/verify.go — Sender interface doc locked the contract to "send emails", contradicting the broader "email, IM, or anything else" on VerifyHandler. AuthHandler comment was copy-pasted from direct. * provider/dev_provider.go — NewDev doc said "for admin user"; just makes the dev oauth2 provider. * middleware/user_updater.go — UserUpdFunc adapter doc said result "is a Handler"; implements UserUpdater. * logger/interface.go — Func adapter doc said "Logf calls f(id)"; actually calls f(format, args...). * token/jwt.go — SendJWTHeader option doc said "instead of cookie"; Set sends header AND cookie. Set doc referenced a "permanent flag" that doesn't exist in the signature (controlled by claims.SessionOnly/Handshake). Get doc oversimplified the XSRF gate. Update/Validate adapter docs said "calls f(id)"; actually f(claims) and f(token, claims). * token/user.go — HashID inline "or empty" was wrong; an empty val never matches reValidSha. * auth.go — BasicAuthChecker doc grammar broken and understated behavior (AdminPasswd is bypassed entirely, not "ignored"); "peak dev provider" typo; AvatarProxy doc was a sentence fragment. All changes mirrored across v1 and v2. Build green, lint clean, go fix clean.
paskal
force-pushed
the
fix/csp-consumer-note
branch
from
0da9c4f to
75471bc
Compare
…curity fix Sweep over the docstrings touched (or adjacent to) PR #290's security work, prompted by Copilot's post-merge review of withSecurityHeaders and an adversarial pass from Codex on the rest of the same surface. All changes are docstring/comment-only; no code, no behavior, no test churn. * withSecurityHeaders CONSUMER NOTE (auth.go, v2/auth.go) — the previous text told consumers HTML custom handlers could fix CSP blocking by "moving scripts/styles to external files served from 'self'", but the wrapper applies default-src 'none' and sandbox, so even self-hosted resources are blocked. New text spells out what the wrapper actually does and gives a concrete relaxed-CSP example. The example list also drops "dev_provider's login page" — that page is served by DevAuthServer on its own HTTP listener, not by handlers Service.Handlers wraps. Replaced with "custom server login pages". * Proxy.Put godoc — was "stores retrieved avatar to avatar.Store. Gets image from user info. Returns proxied url", which omitted the identicon fallback that fires on empty u.Picture, fetch failure, or non-image upstream bytes. Doc now describes that the function silently substitutes an identicon in those cases and returns its proxied URL — the caller is not told the upstream was rejected. * Proxy.Handler godoc — was "returns token routes for given provider", a leftover from a much older shape of the code. Replaced with a description of what Handler actually does today: serves stored avatar bytes by id, sniffs against an allowlist, sets defense headers. * Handler's inline serve-time validation comment — said "validate the bytes really are an image", but Handler reads up to sniffLen bytes and runs them through http.DetectContentType + an allowlist. That is content-type sniffing, not proof of full decodability. Reworded to match. * Proxy.resize godoc — said "validates that the input is a real image", but the no-resize path returns the original bytes after only image.DecodeConfig and the dimension cap; no full decode runs. Split into format/dimension checks (DecodeConfig only) vs full decode (resize path only). * maxAvatarFetchSize constant — mentioned only the remote-URL fetch path; after #290 the cap also bounds PutContent's caller-supplied reader and is the implicit size invariant resize trusts. Doc updated to name both callers. All changes mirrored across v1 and v2.
paskal
force-pushed
the
fix/csp-consumer-note
branch
from
75471bc to
4a9b150
Compare
paskal
added a commit
that referenced
this pull request
May 21, 2026
Pre-existing comment drift in files unrelated to the recent security fix — unrelated docstrings that misled, said the wrong thing, or were copy-paste leftovers. Mirrored across v1 and v2. The security-fix docstring fixes (avatar/avatar.go security comments and auth.go withSecurityHeaders) live in PR #291 and are intentionally NOT included here so the two PRs stay independent. Findings: * avatar/gridfs.go — ID doc claimed MD5 sourced from gridfs; metadata.hash is the sha1 written at Put time. * avatar/bolt.go, avatar/gridfs.go — Put docs and BoltDB type doc claimed these layers "resize" the image; they only copy bytes. Resize happens in Proxy.resize upstream. * avatar/store.go — Migrate doc didn't mention that per-avatar Get/Put/Close errors are logged and skipped, and that the returned count is "ids attempted", not "ids stored". * provider/oauth1.go — initOauth1Handler doc and DEBUG log both said "oauth2". * provider/service.go — Service.Handler doc said "returns auth routes"; it dispatches login/callback/logout. * provider/telegram.go — processUpdates claimed to return an offset (no return value); checkToken doc described an "address or empty string" return shape but the signature is (*token.User, error). * provider/verify.go — Sender interface doc locked the contract to "send emails", contradicting the broader "email, IM, or anything else" on VerifyHandler. AuthHandler comment was copy-pasted from direct. * provider/dev_provider.go — NewDev doc said "for admin user"; just makes the dev oauth2 provider. * middleware/user_updater.go — UserUpdFunc adapter doc said result "is a Handler"; implements UserUpdater. * logger/interface.go — Func adapter doc said "Logf calls f(id)"; actually calls f(format, args...). * token/jwt.go — SendJWTHeader option doc said "instead of cookie"; Set sends header AND cookie. Set doc referenced a "permanent flag" that doesn't exist in the signature (controlled by claims.SessionOnly/Handshake). Get doc oversimplified the XSRF gate. Update/Validate adapter docs said "calls f(id)"; actually f(claims) and f(token, claims). * token/user.go — HashID inline "or empty" was wrong; an empty val never matches reValidSha. * auth.go — BasicAuthChecker doc grammar broken and understated behavior (AdminPasswd is bypassed entirely, not "ignored"); "peak dev provider" typo; AvatarProxy doc was a sentence fragment. All changes mirrored across v1 and v2. Build green, lint clean, go fix clean.
umputun
approved these changes
May 21, 2026
umputun
pushed a commit
that referenced
this pull request
May 21, 2026
Pre-existing comment drift in files unrelated to the recent security fix — unrelated docstrings that misled, said the wrong thing, or were copy-paste leftovers. Mirrored across v1 and v2. The security-fix docstring fixes (avatar/avatar.go security comments and auth.go withSecurityHeaders) live in PR #291 and are intentionally NOT included here so the two PRs stay independent. Findings: * avatar/gridfs.go — ID doc claimed MD5 sourced from gridfs; metadata.hash is the sha1 written at Put time. * avatar/bolt.go, avatar/gridfs.go — Put docs and BoltDB type doc claimed these layers "resize" the image; they only copy bytes. Resize happens in Proxy.resize upstream. * avatar/store.go — Migrate doc didn't mention that per-avatar Get/Put/Close errors are logged and skipped, and that the returned count is "ids attempted", not "ids stored". * provider/oauth1.go — initOauth1Handler doc and DEBUG log both said "oauth2". * provider/service.go — Service.Handler doc said "returns auth routes"; it dispatches login/callback/logout. * provider/telegram.go — processUpdates claimed to return an offset (no return value); checkToken doc described an "address or empty string" return shape but the signature is (*token.User, error). * provider/verify.go — Sender interface doc locked the contract to "send emails", contradicting the broader "email, IM, or anything else" on VerifyHandler. AuthHandler comment was copy-pasted from direct. * provider/dev_provider.go — NewDev doc said "for admin user"; just makes the dev oauth2 provider. * middleware/user_updater.go — UserUpdFunc adapter doc said result "is a Handler"; implements UserUpdater. * logger/interface.go — Func adapter doc said "Logf calls f(id)"; actually calls f(format, args...). * token/jwt.go — SendJWTHeader option doc said "instead of cookie"; Set sends header AND cookie. Set doc referenced a "permanent flag" that doesn't exist in the signature (controlled by claims.SessionOnly/Handshake). Get doc oversimplified the XSRF gate. Update/Validate adapter docs said "calls f(id)"; actually f(claims) and f(token, claims). * token/user.go — HashID inline "or empty" was wrong; an empty val never matches reValidSha. * auth.go — BasicAuthChecker doc grammar broken and understated behavior (AdminPasswd is bypassed entirely, not "ignored"); "peak dev provider" typo; AvatarProxy doc was a sentence fragment. All changes mirrored across v1 and v2. Build green, lint clean, go fix clean.
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
3 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.
Docstring sweep over the surface touched by (or adjacent to) PR #290's
security work, prompted by Copilot's post-merge review of
withSecurityHeadersand a Codex adversarial pass over the rest of thesame surface. All changes are docstring/comment-only — no code, no
behavior change, no test churn.
Six fixes, mirrored across v1 and v2:
withSecurityHeadersCONSUMER NOTE (auth.go,v2/auth.go) — theprevious text told consumers HTML custom handlers could fix CSP blocking
by "moving scripts/styles to external files served from 'self'", but
the wrapper applies
default-src 'none'andsandbox, so evenself-hosted resources are blocked. The example list also named "the
dev_provider's login page", which is served by
DevAuthServeron itsown HTTP listener and is not wrapped by
Service.Handlers().Replaced with "custom server login pages" and a concrete relaxed-CSP
example.
Proxy.Put— was "stores retrieved avatar to avatar.Store. Getsimage from user info. Returns proxied url", which omitted the identicon
fallback that fires on empty
u.Picture, fetch failure, or non-imageupstream bytes. Doc now describes that the function silently substitutes
an identicon in those cases — the caller is not told the upstream was
rejected.
Proxy.Handlergodoc — was "returns token routes for givenprovider", a leftover from a much older shape of the code. Replaced
with what Handler actually does: serves stored avatar bytes by id,
sniffs against an allowlist, sets defense headers.
Handler inline serve-time comment — said "validate the bytes really
are an image", but Handler reads up to
sniffLenbytes and runs themthrough
http.DetectContentType+ the allowlist. That is content-typesniffing, not proof of full decodability. Reworded to match.
Proxy.resizegodoc — said "validates that the input is a realimage", but the no-resize path returns the original bytes after only
image.DecodeConfigand the dimension cap; no full decode runs. Splitinto format/dimension metadata checks (DecodeConfig only) vs full
decode (resize path only).
maxAvatarFetchSizeconstant — mentioned only the remote-URL fetchpath; after fix(avatar): prevent stored XSS via content-type spoofing #290 the cap also bounds
PutContent's caller-suppliedreader and is the implicit size invariant
resizetrusts. Doc nownames both callers.