Repository navigation
Scope feed share tokens to their owners. Closes #1806 - #1810
regulartim merged 5 commits into
Conversation
regulartim
left a comment
regulartim
left a comment
There was a problem hiding this comment.
Hey @ABHIGYA0205 ! Looks good all in all, thank you! :) Could you take a look at the tests in tests/api/views/test_feeds_advanced_view.py? Some of them still sign with the hardcoded "greedybear-feeds" salt. We should also update them, right?
|
|
||
| def share_token_salt_for_user(user_id: int) -> str: | ||
| """Derive a signing salt scoped to the user who created a share token.""" | ||
| return f"{SHARE_TOKEN_SALT}:{user_id}" | ||
|
|
||
|
|
There was a problem hiding this comment.
A function in a constants module is a bit out of place, don't you think? Can you think of a better place?
There was a problem hiding this comment.
I moved share_token_salt_for_user() into api/share_tokens.py and updated the view and serializer to import it from there. greedybear/consts.py now just holds the salt and max-age constants.
…owner-scoping-v2 # Conflicts: # tests/api/views/test_feeds_advanced_view.py
|
Yes, I updated those tests to use the user-specific salt helper instead of hardcoding "greedybear-feeds". After merging develop, I reran the focused feed tests locally, and all 85 passed. Let me know if there’s anything else you’d like me to adjust. |
regulartim
left a comment
regulartim
left a comment
There was a problem hiding this comment.
Hey @ABHIGYA0205 ! I know this is very nit-picky, but I moved the method to models.py . I think that's the best place for it. What do you think? If you are fine with this, we can merge! :)
|
yeah @regulartim , I think that makes sense. Since the salt is tied to the token’s owner, keeping it on ShareToken feels like the right place. I’m happy with this change. You can merge it ! :) |
Description
The feed-share endpoint can produce identical tokens when different users request the same feed filters within the same second. The
reasonparameter is excluded from the signed data, andShareToken.token_hashis unique. As a result, a later user’s request can reuse a token record owned by the first user, preventing the later user from listing or revoking that token.The signing salt is now derived from the creating user’s ID, and token verification uses the owner ID stored on the
ShareTokenrecord. A fixed-time regression test uses two non-staff users and verifies distinct tokens, correct ownership, per-user listings, cross-user revoke denial, and each owner’s ability to revoke their own token.Compatibility: Pre-existing links signed with the old global salt will no longer validate after this change. The maintainer confirmed in #1806 that this invalidation is acceptable for the next major release.
Validation: The PR backend CI lint and Python test steps passed. The two focused modules also passed locally (81 tests).
Related issues
Closes #1806 — Same-second share requests can return a token owned by another user.
Type of change
Checklist
Please complete this checklist carefully. It helps guide your contribution and lets maintainers verify that all requirements are met.
Formalities
<feature name>. Closes #999develop.develop.Docs and tests
Ruff) gave 0 errors. If you have correctly installed pre-commit, it does these checks and adjustments on your behalf.GUI changes
Ignore this section if you did not make any changes to the GUI.
Review process