docs: package-wide comment sweep#292
Merged
Merged
Conversation
Coverage Report for CI Build 26202899073Coverage increased (+0.03%) to 85.425%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
415d7ca to
fb99a2a
Compare
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.
fb99a2a to
8de06b4
Compare
umputun
approved these changes
May 21, 2026
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.
Package-wide comment sweep. Two scopes in one commit:
running the same audit prompt over the v1 package that originally turned up
the issues post-fix(avatar): prevent stored XSS via content-type spoofing #290.
API-contract size. Several of those were factually loose; tighter prose has
less surface to be wrong.
Net 28 files (v1 + v2 mirror). Line count drops by ~30. Doc-only — no symbol
changes, no test changes. Build green, lint clean,
go fix ./...clean on bothmodules.
Note on overlap with #291: #291 fixed a subset of the security-adjacent
docstrings touched here. This PR covers everything #291 covers plus the wider
package. If this lands first, #291 can be closed; otherwise we rebase.
Drift fixes
avatar/:gridfs.go—IDdoc claimed it returns MD5 sourced from GridFS; metadata isthe SHA-1 written at
Puttime.bolt.go,gridfs.go—Putdocs and theBoltDBtype doc claimed thesestore layers "resize" the image; they copy bytes verbatim. Resize happens
upstream in
Proxy.resize.store.go—Migratedoc didn't mention per-avatarGet/Put/Closeerrorsare logged-and-skipped, and the returned count is "ids attempted", not "ids
stored".
provider/:oauth1.go—initOauth1Handlergodoc and its[DEBUG]log both said"oauth2"; function and protocol are OAuth 1.
service.go—Service.Handlerdoc said it "returns auth routes"; itdispatches login/callback/logout.
telegram.go—processUpdatesclaimed to return an offset (no return value);checkTokendoc described an "address or empty string" return shape but thesignature is
(*token.User, error).verify.go—Senderinterface doc locked the contract to "send emails",contradicting the broader "email, IM, or anything else" comment on
VerifyHandlerupstream.AuthHandlercomment was copy-pasted fromprovider/direct.go.dev_provider.go—NewDevdoc said "for admin user"; it makes the devoauth2 provider, admin role is set separately.
middleware/andlogger/:user_updater.go—UserUpdFuncadapter doc said the result "is aHandler"; it implements
UserUpdater.interface.go—Funcadapter doc said "Logf calls f(id)"; it callsf(format, args...).token/:jwt.go—SendJWTHeaderoption doc said "instead of cookie";Setsendsheader and cookie (header path explicitly falls through).
Setdocreferenced a "permanent flag" that doesn't exist in the signature (controlled
by
claims.SessionOnly/Handshake).Getdoc oversimplified the XSRF gate(skipped when
DisableXSRF, method inXSRFIgnoreMethods, or claims carry nouser).
Update/Validateadapter docs said "callsf(id)"; actuallyf(claims)andf(token, claims).user.go—HashIDinline "or empty" was wrong; an empty val never matchesreValidSha.auth.go:BasicAuthCheckerdoc grammar was broken and understated behavior — when achecker is set,
AdminPasswdis bypassed entirely, not "ignored".// peak dev providertypo for "peek".AvatarProxydoc was a sentence fragment.Trims (rationale prose from #290 → API contract)
avatar/avatar.go:maxAvatarFetchSize,maxAvatarPixels— dropped the "Telegram caps at 5 MiB"/ "100 KB declaring 65535x65535 px" rationale, kept the contract.
Proxy.Put— now describes the identicon fallback (which the old doc didn'tmention at all).
Proxy.Handlergodoc — replaced the stale "returns token routes" with whatthe function does today.
image"; it's content-type sniffing against an allowlist on the first
sniffLenbytes, not a full decode.Proxy.resize— split the doc cleanly between metadata-only check(
DecodeConfigpath) and full decode (resize path). Dropped the inlineexample.
safeImgContentType— kept the allowlist and SVG-excluded note, dropped the"future scriptable image/*" defensive rationale.
auth.go:withSecurityHeaders— dropped the bullet-list explanation of standard HTTPheaders. Kept the CONSUMER NOTE (real footgun) but tightened it.
All changes mirrored across v1 and v2.