fix(notifications): require a valid code, and escape the upstream path - #91
Merged
Conversation
Closes half of #90. Two defects in the notifications handler. Authentication. The guard accepted a bare `user` body field in place of a code, so the 401 was only reachable when `user` was absent. Supplying it was enough to be served that account's notifications with no authentication at all. A valid code is now required before anything else is read. Path building. username, filter, since and limit were interpolated into the upstream path through Template(), which emulates JS string coercion and does no URL encoding. A value carrying / ? or # was therefore re-parsed as URL structure once the string became a Uri, reaching a different upstream endpoint with this service's credentials attached. That is what left the api-proxy per-path allowlist acting as a security control rather than routing hygiene. NotificationsPath() now escapes every segment and rejects dot segments, exactly as PostTipsPath() already does for the tips handlers, and for the same reason. Escaping is a no-op for real values: account names, filter names, notification ids and integer limits are all unreserved characters, so live requests are byte-identical. NOT changed, deliberately: a caller with a valid code can still name another account via `user`. Decks depends on it. Its notifications column is built from a free-text account search box and passes settings.username alongside the signed-in user's code (vision-web deck-notifications-column.tsx), so removing the override would break a shipped feature. This narrows the exposure from anyone on the internet to any signed-in user; whether that should be narrowed further is a product decision tracked in #90. Tests mirror PostTipsPathTests: real requests unchanged, structural characters cannot escape their segment, dot segments rejected, and query values cannot append parameters of their own. No dotnet SDK on the machine this was written on, so the build and tests were not run locally. CI runs both on pull_request.
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
PR Summary by QodoSecure notification authentication and upstream path construction
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1. Notifications authorization remains untested
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+14
to
+17
| public class NotificationsPathTests | ||
| { | ||
| [Fact] | ||
| public void RealRequestsAreUnchanged() |
There was a problem hiding this comment.
1. notifications authorization remains untested 📎 Requirement gap ☼ Reliability
The added regression suite exercises only URI construction and does not verify unauthorized requests or mismatched authenticated identities. The authorization defects can therefore recur without the required automated test failure.
Agent Prompt
## Issue description
The new tests cover notification path escaping but omit the required authorization regressions for body-only and mismatched account access.
## Issue Context
Add handler-level tests proving that missing or invalid codes return 401 even when `user` is supplied, and that a body account differing from the validated account is rejected or cannot select another account's notifications.
## Fix Focus Areas
- dotnet/EcencyApi.Tests/NotificationsPathTests.cs[14-97]
- dotnet/EcencyApi/Handlers/PrivateApi.UserData1.cs[55-78]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Member
Author
There was a problem hiding this comment.
Fair, and fixed. The suite did only cover URI construction, so both original defects could have recurred silently.
ResolveNotificationsTarget() now holds the authorization decision as a pure function, with NotificationsAuthorizationTests covering it directly:
- no valid code is unauthorized even when an account is named, which is the exact bypass that existed
- a valid code alone serves that account's complete feed
- naming your own account is still a self view, case-insensitively
- naming another account is permitted but never sets full scope, including for near-miss names like
good-karmorgood_karma
IsTruthy stays in the handler so the port keeps its JS truthiness parity while the decision itself stays pure.
CI: 214 passed, 0 failed.
Follow-up to the auth fix in this PR, and the product half of #90. Decks builds a notifications column for an arbitrary account and passes that name alongside the signed-in user's own code. That stays supported: notifications are largely public data and the column exists for that reason. But the feed also carries Ecency-only activity that is not public. Favorites and bookmarks reveal who a user follows and what they saved, and Points transfers, streaks and the monthly/weekly aggregates exist nowhere on chain. So a request for SOMEONE ELSE's notifications now carries scope=public, which enotify restricts to chain-derived types (ecency/enotify-py#21). This service is the only layer that can make that call, because it is the only one that has validated who is asking. The comparison is case-insensitive, and a request for your own account is unaffected: without the flag the upstream path is byte-identical to what it was before scope existed. scope is derived from the validated code and never read from the body, so a caller cannot ask for a wider view than they are entitled to. NotificationsPath now assembles query values through a list, which keeps the original `?` then `&` ordering while making the appended parameter unambiguous. Tests cover the flag off and on, joining against existing since/limit values, and that a since value trying to smuggle its own scope parameter is escaped into a literal rather than overriding the real one. Still no dotnet SDK locally, so CI remains the gate.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe notifications handler now requires a valid code, resolves self and cross-account targets, escapes URL values, rejects dot segments, and uses ChangesNotifications authentication and scope
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The PR tightens notification authorization and safely escapes upstream path values without any actionable merge-blocking risk remaining beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Client
participant Notifications
participant NotificationsPath
participant UpstreamNotificationsApi
Client->>Notifications: Submit notifications request
Notifications->>Notifications: Validate code and resolve target
Notifications->>NotificationsPath: Build escaped URL with full-scope flag
NotificationsPath-->>Notifications: Return URL or null
Notifications->>UpstreamNotificationsApi: Request URL with optional internal token
UpstreamNotificationsApi-->>Notifications: Return notifications response
Notifications-->>Client: Return response or authorization error
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
added 2 commits
September 1, 2026 16:26
Follows the enotify review. enotify now defaults to chain-derived activity only and requires scope=full to widen, because it performs no authentication of its own and its host was reachable from the public internet, so an opt-in restriction protected nothing against a caller who simply omitted the parameter. This side flips to match. A self-view asks for scope=full; a cross-account view sends no scope parameter at all, which is the safe direction: any request that never reaches this handler now gets the restricted feed rather than the whole one. Behaviour for users is unchanged in both directions. Deploy this before the enotify change: scope=full is ignored until enotify ships, so there is no window where a user loses their own favorites, bookmarks or aggregates. The forgery test now asserts the opposite direction too: a since value carrying `scope=full` is escaped into a literal and, with the flag off, no real scope parameter is appended for it to piggyback on.
Two review findings. enotify now requires X-Ecency-Internal-Token alongside scope=full, because a query parameter cannot gate private data on a service with no authentication of its own. This side presents it, from ENOTIFY_INTERNAL_TOKEN, only on a self-view. enotify fails closed, so a missing or wrong token costs that user their own private activity rather than exposing anyone else's. Qodo was right that the authorization itself had no test: the suite covered URI construction only, so both original defects could recur silently. ResolveNotificationsTarget() now holds the decision as a pure function and is tested directly: no valid code is unauthorized even when an account is named, which is the exact bypass; a code alone serves that account's complete feed; the self-view comparison is case-insensitive; naming another account is still permitted but never sets full scope, including for near-miss names. IsTruthy stays in the handler so the port keeps its JS truthiness parity while the decision stays pure. Still no dotnet SDK locally, so CI remains the gate.
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
1 participant
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 half of #90. Two defects in
PrivateApi.Notifications.1. A code was not actually required
The 401 was only reachable when
userwas absent. Supplying it satisfied the guard and became the account queried, so the endpoint served notifications with no authentication at all.A valid code is now required before anything else is read, matching
UnreadNotificationsdirectly below.2. The upstream path was built by unescaped interpolation
username,filter,sinceandlimitall went into the path throughTemplate(), which emulates JS string coercion and performs no URL encoding. A value carrying/,?or#was therefore re-parsed as URL structure once the string became aUri, reaching a different upstream endpoint with this service's credentials attached.That is what left the api-proxy per-path allowlist acting as a security control rather than as routing hygiene.
NotificationsPath()now escapes every segment and rejects dot segments, exactly asPostTipsPath()already does for the tips handlers and for the same reason. Escaping is a no-op for real values: account names, filter names, notification ids and integer limits are all unreserved characters, so live requests are byte-identical.3. Cross-account views are downgraded to the restricted feed
A caller with a valid code can still name another account via
user. Decks depends on this: its notifications column is built from a free-text account search box and passessettings.usernamealongside the signed-in user's own code. Notifications are largely public data and that column exists for that reason, so it stays.But the feed also carries Ecency-only activity that is not public: favorites and bookmarks reveal who a user follows and what they saved, and Points transfers, streaks and the aggregates exist nowhere on chain.
So only a self-view asks for
scope=full. A cross-account view sends no scope parameter, and enotify (ecency/enotify-py#21) defaults to chain-derived activity only.Omitting the parameter is deliberately the safe direction: any request that never reaches this handler gets the restricted feed rather than the whole one.
scopeis derived from the validated code and never read from the body, so a caller cannot ask for a wider view than they are entitled to.Companion: ecency/vision-web#1709 hides the now-restricted types in the Decks picker so nobody builds a column that cannot load.
Deploy order
Ship this before ecency/enotify-py#21.
scope=fullis ignored until enotify's change lands, so there is no window where a user loses their own favorites, bookmarks or aggregates.Tests
NotificationsPathTestsmirrorsPostTipsPathTests:Uridecodes%2Eback to.before removing themlimitstill joins with&whensinceis present and?when it is not, preserving the original URL shape for existing paging clientsVerification note
There is no dotnet SDK on the machine this was written on, so the build and tests were not run locally. CI is the gate, and it is green:
Failed: 0, Passed: 206. Escaping expectations were checked against the unreserved-character set independently rather than assumed.Summary by CodeRabbit