Skip to content

fix(admin): correct CORS configuration guidance DEV-2471#7284

Open
jnm wants to merge 3 commits into
mainfrom
fix-cors-configuration-guidance
Open

fix(admin): correct CORS configuration guidance DEV-2471#7284
jnm wants to merge 3 commits into
mainfrom
fix-cors-configuration-guidance

Conversation

@jnm

@jnm jnm commented Jul 20, 2026

Copy link
Copy Markdown
Member

🗒️ Checklist

  1. run linter locally
  2. update developer docs (API, README, inline, etc.), if any
  3. for user-facing doc changes create a Zulip thread at #Support Docs Updates, if any
  4. draft PR with a title <type>(<scope>)<!>: <title> DEV-1234
  5. assign yourself, tag PR: at least Front end and/or Back end or workflow
  6. fill in the template below and delete template comments
  7. review thyself: read the diff and repro the preview as written
  8. open PR & confirm that CI passes & request reviewers, if needed
  9. act on any greptile review below a 5/5 score or leave comment explaining why you won't
  10. delete this checklist section from the final squash commit before merging
  11. fast-forward Linear issue to Done status after merging, as no QA team involvement is needed

👷 Description for instance maintainers

Fix CORS configuration guidance in the Django Admin interface so that it correctly says NOT to add default ports 80 or 443 for HTTP or HTTPS, respectively. Add a validator to prevent saving new origins if this guidance is not heeded.

👀 Preview steps

  1. Go to /admin/external_integrations/corsmodel/add/
  2. Check that the help text includes "Do not include the default port for the scheme (80 for http, 443 for https)"
  3. Attempt to add http://some-origin:80 and https://some-origin:443: both should fail
  4. Attempt to add an oddball http://some-origin:443: it should succeed

jnm added 2 commits July 20, 2026 14:16
…and add a validator to prevent standard ports 80 or 443 from being
added, since those are never to be sent by clients per spec
@jnm jnm changed the title feat(admin): fix CORS configuration guidance DEV-2471 fix(admin): correct CORS configuration guidance DEV-2471 Jul 20, 2026
@jnm jnm self-assigned this Jul 20, 2026
@jnm

jnm commented Jul 20, 2026

Copy link
Copy Markdown
Member Author

@greptileai please review

@greptile-apps

greptile-apps Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes CORS configuration guidance in the Django Admin by correcting the help text to say not to include default ports (80/443), adds a clean() validator that enforces this rule at save time, and changes the origin-matching query from an exact match to a case-insensitive (iexact) lookup.

  • Help text & migration: 0002_do_nothing.py is updated in-place to reflect the revised cors field help text, clarifying that standard ports must be omitted.
  • Validator: CorsModel.clean() iterates over two (scheme, port) pairs using tuple unpacking and raises a ValidationError when a default port is detected. This guard only fires during Django's full_clean() path (admin forms); direct objects.create() calls bypass it, which is expected Django behaviour.
  • Signal & tests: cors_allow_external_sites now short-circuits on missing origin and matches case-insensitively; four new tests cover case-insensitivity, port rejection, and the happy path.

Confidence Score: 5/5

Safe to merge — changes are tightly scoped to admin-side help text, a save-time validator, and a case-insensitive origin lookup, with no database schema changes and no risk of breaking existing stored origins.

All three code changes are correct: the tuple-of-tuples iteration in clean() unpacks as intended, the iexact lookup is a strictly additive improvement, and the guard for missing origin headers prevents a needless DB query. The new tests exercise the key scenarios.

No files require special attention.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Admin form: save CorsModel] --> B[Django full_clean]
    B --> C[clean_fields: blank/null check]
    C --> D[CorsModel.clean]
    D --> E{starts with http: AND ends with :80?}
    E -- Yes --> F[ValidationError: default port 80]
    E -- No --> G{starts with https: AND ends with :443?}
    G -- Yes --> H[ValidationError: default port 443]
    G -- No --> I[Save to DB]

    J[Incoming CORS request] --> K[cors_allow_external_sites signal]
    K --> L{Origin header present?}
    L -- No --> M[return False]
    L -- Yes --> N[CorsModel.objects.filter cors__iexact=origin .exists]
    N -- Found --> O[Allow: set Access-Control-Allow-Origin header]
    N -- Not found --> P[Deny]
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[Admin form: save CorsModel] --> B[Django full_clean]
    B --> C[clean_fields: blank/null check]
    C --> D[CorsModel.clean]
    D --> E{starts with http: AND ends with :80?}
    E -- Yes --> F[ValidationError: default port 80]
    E -- No --> G{starts with https: AND ends with :443?}
    G -- Yes --> H[ValidationError: default port 443]
    G -- No --> I[Save to DB]

    J[Incoming CORS request] --> K[cors_allow_external_sites signal]
    K --> L{Origin header present?}
    L -- No --> M[return False]
    L -- Yes --> N[CorsModel.objects.filter cors__iexact=origin .exists]
    N -- Found --> O[Allow: set Access-Control-Allow-Origin header]
    N -- Not found --> P[Deny]
Loading

Reviews (3): Last reviewed commit: "D'oh" | Re-trigger Greptile

Comment thread kobo/apps/external_integrations/models.py Outdated
@jnm

jnm commented Jul 20, 2026

Copy link
Copy Markdown
Member Author

@greptileai please review again

@jnm jnm added the Back end label Jul 20, 2026
@jnm
jnm marked this pull request as ready for review July 20, 2026 21:53
@jnm
jnm requested a review from noliveleger as a code owner July 20, 2026 21:53
@jnm
jnm removed the request for review from noliveleger July 20, 2026 21:53
@platreth
platreth self-requested a review July 21, 2026 07:04

@platreth platreth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants