Skip to content

fix(Identities): Sanitize identifiers#6024

Merged
emyller merged 14 commits intomainfrom
fix/sanitize-identifiers
Sep 12, 2025
Merged

fix(Identities): Sanitize identifiers#6024
emyller merged 14 commits intomainfrom
fix/sanitize-identifiers

Conversation

@emyller
Copy link
Copy Markdown
Contributor

@emyller emyller commented Sep 6, 2025

Contributes to #5490.

Rewrites how the endpoint /api/v1/identities/ handles input, properly validating data, and ultimately rejecting invalid identifiers.

This includes a state-only migration (safe to apply without database changes).

@vercel
Copy link
Copy Markdown

vercel Bot commented Sep 6, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
docs Ignored Ignored Preview Sep 11, 2025 4:48pm
flagsmith-frontend-preview Ignored Ignored Preview Sep 11, 2025 4:48pm
flagsmith-frontend-staging Ignored Ignored Preview Sep 11, 2025 4:48pm

@github-actions github-actions Bot added api Issue related to the REST API fix labels Sep 6, 2025
@github-actions github-actions Bot added fix and removed fix labels Sep 8, 2025
@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 8, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.00%. Comparing base (a809f7c) to head (fa5b39e).
⚠️ Report is 8 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6024   +/-   ##
=======================================
  Coverage   97.99%   98.00%           
=======================================
  Files        1275     1277    +2     
  Lines       44892    44981   +89     
=======================================
+ Hits        43993    44083   +90     
+ Misses        899      898    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread api/environments/identities/views.py Outdated
@emyller emyller self-assigned this Sep 8, 2025
@emyller emyller marked this pull request as ready for review September 8, 2025 23:46
@emyller emyller requested a review from a team as a code owner September 8, 2025 23:46
@emyller emyller requested review from gagantrivedi and removed request for a team September 8, 2025 23:46
@github-actions github-actions Bot added fix and removed fix labels Sep 8, 2025
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Sep 8, 2025

Docker builds report

Image Build Status Security report
ghcr.io/flagsmith/flagsmith-api-test:pr-6024 Finished ✅ Skipped
ghcr.io/flagsmith/flagsmith-e2e:pr-6024 Finished ✅ Skipped
ghcr.io/flagsmith/flagsmith-frontend:pr-6024 Finished ✅ Results
ghcr.io/flagsmith/flagsmith-api:pr-6024 Finished ✅ Results
ghcr.io/flagsmith/flagsmith:pr-6024 Finished ✅ Results
ghcr.io/flagsmith/flagsmith-private-cloud:pr-6024 Finished ✅ Results

Copy link
Copy Markdown
Member

@gagantrivedi gagantrivedi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread api/environments/identities/views.py Outdated
@github-actions github-actions Bot added fix and removed fix labels Sep 9, 2025
@emyller
Copy link
Copy Markdown
Contributor Author

emyller commented Sep 9, 2025

Looks good, but we also need to update: (...) and add tests for (...)

@gagantrivedi Thanks! Unpacking the multiple changes:

  1. Identifier validation is now also implemented in the edge-identifiers endpoint (e49a57d).
  2. Tests were improved in 8f8167f.
  3. Regarding a couple of the references, I prefer not to make changes because:
    • They are deprecated endpoints.
    • Their associated URLs (ref) already use a subset of the regular expression introduced to validate identifiers — let's not expand it.

@github-actions github-actions Bot added fix and removed docs Documentation updates labels Sep 11, 2025
cursor[bot]

This comment was marked as outdated.

@emyller
Copy link
Copy Markdown
Contributor Author

emyller commented Sep 11, 2025

Fair enough, but I guess we missed another SDK endpoint (that is no deprecated and should get some tests) https://github.com/Flagsmith/flagsmith/blob/main/api/environments/identities/traits/views.py#L182

You're right, added tests here: 1ba40b9. I also did a search through the API and it looks like these were all views affected by the model change. Thanks for pointing them out so far!

@github-actions github-actions Bot added docs Documentation updates fix and removed fix docs Documentation updates labels Sep 11, 2025
@emyller emyller force-pushed the fix/sanitize-identifiers branch from 5653858 to 6369c8b Compare September 11, 2025 01:41
@github-actions github-actions Bot added docs Documentation updates fix and removed fix docs Documentation updates labels Sep 11, 2025
Comment thread api/tests/integration/edge_api/identities/test_edge_identity_viewset.py Outdated
Comment thread api/tests/conftest.py Outdated
@github-actions github-actions Bot added docs Documentation updates fix and removed fix docs Documentation updates labels Sep 11, 2025
@emyller emyller merged commit dfb41c6 into main Sep 12, 2025
31 checks passed
@emyller emyller deleted the fix/sanitize-identifiers branch September 12, 2025 12:57
emyller added a commit that referenced this pull request Sep 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api Issue related to the REST API fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants