fix(comments): use shared IP hash instead of local hardcoded-pepper copy - #74
Merged
royalpinto007 merged 2 commits intoAug 7, 2026
Conversation
Both /status/[token] and /unsubscribe/[token] carry a private token in the URL and are linked from outbound email, same as /edit/[token]. Add them to the disallow list and normalize /admin to have a trailing slash like the rest. Fixes AgentPostmortem#57
app/api/comments/route.ts had its own hashIp/getIp instead of the lib/utils/hash helpers every other route uses. Three problems: it fell back to the literal string "default-pepper" when IP_HASH_PEPPER was unset instead of failing loudly, it used SHA256(ip + pepper) instead of HMAC-SHA256(pepper, ip) so the same visitor hashed differently here vs. other endpoints, and it never checked x-real-ip so proxies without x-forwarded-for collapsed every commenter into one rate-limit bucket. Swap in hashIp/getClientIp from lib/utils/hash and log the error in the catch block instead of swallowing it. Existing comments.ip_hash rows were written with the old scheme and won't match going forward. That's fine since the column is only used for rate limiting/abuse tracing, not identity, but a maintainer may want to clear the rate limit table on deploy.
|
And it's in. Thanks @waterlemonnn. Good, focused change. Exactly the kind of thing agentpostmortem needs. If agentpostmortem is useful to you, a star goes a long way. Happy to see more from you whenever you have the time. |
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
2 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.
Closes #32.
app/api/comments/route.tshad its ownhashIp/getIpinstead of thelib/utils/hashhelpers every other route uses (e.g.app/api/upload/presign/route.ts). Three problems, per the issue:"default-pepper"whenIP_HASH_PEPPERwas unset instead of failing loudly like the sharedhashIp()does.SHA256(ip + pepper)instead ofHMAC-SHA256(pepper, ip), so the same visitor got a different hash here vs. every other endpoint, breaking cross-endpoint rate limiting/dedup.x-real-ip, so behind a proxy that only sets that header, everyone collapsed into the single"unknown"bucket.This swaps in
hashIp/getClientIpfromlib/utils/hashand drops the local copies + the now-unusedcreateHashimport. Also added aconsole.errorin the catch block so a thrown "pepper not set" error doesn't get swallowed into the generic 500 without a trace.Migration note: existing
comments.ip_hashrows were written with the old scheme and won't match the new one going forward. That's fine since the column is only used for rate limiting and abuse tracing, not identity — no backfill attempted. A maintainer may want to clear the rate-limit rows on deploy.Verified locally:
npx tsc --noEmitcleannpm run lintcleannpx vitest run— 87/87 passinghashIp()throws whenIP_HASH_PEPPERis unset (caught by the route's try/catch, logged, returns 500) rather than silently accepting the comment