fix: ensure alias can only be linked to a single address#630
Conversation
Adds a duplicate check in the alias writer's verify() step to return a clear error instead of relying on the DB primary key constraint. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2bb4b30 to
86030c7
Compare
ChaituVR
left a comment
There was a problem hiding this comment.
I think we should add Unique index on DB instead of a check on application, wyt?
Adds a check that rejects alias creation if the alias address is already associated with a different parent address, preventing one alias from acting on behalf of multiple addresses. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
86030c7 to
b9e5ff4
Compare
Added |
There was a problem hiding this comment.
Pull request overview
This PR closes snapshot-labs/workflow#767 by enforcing a 1:1 relationship between an alias address and its parent address (preventing one alias from acting on behalf of multiple identities), using both an application-level pre-check and a DB-level uniqueness constraint.
Changes:
- Updated alias writer verification to reject alias creation when the alias is already linked (either to the same address or a different one) using a single lookup by
alias. - Added a
UNIQUE KEYon thealiases.aliascolumn in the test schema to enforce 1:1 alias ownership at the database level. - Added an integration test covering the “alias already linked to another address” rejection path (and adjusted fixtures to satisfy the new uniqueness constraint).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/writer/alias.ts |
Consolidates alias existence checks into a single query by alias and introduces new rejection message for cross-address conflicts. |
test/schema.sql |
Adds a unique index on aliases.alias to enforce one alias per table globally. |
test/integration/helpers/alias.test.ts |
Adds integration coverage for rejecting alias reuse across different parent addresses. |
test/fixtures/alias.ts |
Updates fixture data to avoid collisions under the new unique index. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
0ebe3fc to
03c0842
Compare
ee57600 to
0beb198
Compare
Allow the same (address, alias) pair to be submitted again to refresh the expiration. Uses ON DUPLICATE KEY UPDATE to reset created timestamp.
0beb198 to
d7424ee
Compare
|
@copilot fix the merge conflict |
Co-authored-by: wa0x6e <495709+wa0x6e@users.noreply.github.com>
Co-authored-by: wa0x6e <495709+wa0x6e@users.noreply.github.com>
Co-authored-by: wa0x6e <495709+wa0x6e@users.noreply.github.com>
This reverts commit 6bf2457. Co-authored-by: wa0x6e <495709+wa0x6e@users.noreply.github.com>
Fixed in 311c88d. Merged the updated base branch (which added alias renewal via ON DUPLICATE KEY UPDATE) into our branch. The combined result:
|
|
@copilot fix the merge conflict |
Co-authored-by: wa0x6e <495709+wa0x6e@users.noreply.github.com>
Fixed in afb57c4. The base branch |
jest.mock doesn't work with jest-environment-node-single-context since the mysql module is already loaded by globalSetup.
|
Looks good to me, except there are currently 120 alias addresses linked to multiple owners in the DB How do we handle this before running the |
|
We did not prevent multiple alias before this PR, and allows multiple addresses using the same alias. Aliases generation on our UI should generate a new alias each time, and never reuse, so those duplicate probably come from people who set them via API, and use this a kind of delegation system, the duplicate alias was a feature. Changing the behavior to be unique will probably break their integration. On the other hand, with the new UI page for creating alias, with the alias address only set via the URL, anybody can set an alias address they control in the url, and share this page. Once an user sign this page without paying attention (because this alias address is new to him), he hereby authorize this alias to act on his behalf. Merging this PR may break integration for the existing integrator using it as a delegation system, but not merging it may expose the alias to phishing attacks. The patch should probably be on the UI alias creation page, and make the page params more robust by adding a verification message signed the alias wallet, and be time sensitive (more robust way would be to also include the aliased address in the verification message) |
|
Oh yes i agree. but what i meant was that if we run |
|
Maybe not go with the PR, as duplication is widely used as a feature |
|
But the phishing attack you mentioned is super valid 🙈 Maybe we can ignore uniqueness at SQL level and still check in app level? so weird that so many already use the alias system in wrong way. 🙈 not sure what's the right way to handle this. maybe we need need two signatures (one from user and one from alias as you suggest) which are signed at same time, to prove both belong to same user? |
|
Duplicates have been removed from the database, they were not genuine. Database has also been updates with the new index |
Summary
An alias could previously be linked to multiple addresses, allowing different users to claim the same alias. This PR adds a server-side check in
verify()to reject alias creation when the alias is already linked to a different address, while still allowing the same address to renew its existing alias.Changes
verify(): rejects when alias is linked to a different address (single SQL query withAND address != ?)action(): usesON DUPLICATE KEY UPDATEfor renewals (from master via PR fix: allow alias renewal by re-submitting existing pair #629)UNIQUE KEYonaliases.aliasin test schemaUNIQUE KEYcollisionverify()now queries the DBDatabase migration
Run the following on the production database to enforce uniqueness at the DB level:
Test plan
yarn test:unit— alias unit tests pass with mocked DByarn test:integration— alias integration tests pass, including new cross-address rejection testyarn test:e2e— existing e2e tests unaffected