Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPIKE] Research prevention of mismatched api urls #2414

Open
exalate-issue-sync bot opened this issue Nov 26, 2024 · 4 comments
Open

[SPIKE] Research prevention of mismatched api urls #2414

exalate-issue-sync bot opened this issue Nov 26, 2024 · 4 comments
Assignees

Comments

@exalate-issue-sync
Copy link

exalate-issue-sync bot commented Nov 26, 2024

Identify potential solutions to avoid committing front end code that requests the api with urls that are correct except for missing a / at the end. These are easy to miss because the api redirects and the app behaves correctly. Consider automated solutions and change of process solutions.

Upon review, follow up tickets will be made for any solutions we decide to act on.

QA Notes

null

DEV Notes

Note: time box to “2” points

Design

null

See full ticket and images here: FECFILE-1843

Copy link
Author

Sasha Dresden commented: So I did some research on this and there are several things I have found out.

  • Django defaults to having a trailing slash when you use their views.

It is possible to override this, but we don’t do so very often. Looking at our code base and the /api/docs/ the only place we are doing that is for the oidc calls. I’m not quite sure why we don’t have trailing slashes for them. Perhaps they prefer no trailing slash? Or just whoever set up these routes didn’t add them. But I tried making the mock_oidc calls with a trailing slash and they all work. I did not test with the non-mocked oidc calls, as that would require a push to dev, but I imagine it would behave the same.

  • An easy way to make sure redirects don’t happen is to turn off APPEND_SLASH in django. This feature is what is handling the redirects for when the front-end doesn’t have the correct trailing slash. It does a redirect with the trailing slash. This is robust as it allows people to make mistakes by forgetting to type the trailing slash and it will still work properly, but it does come with a performance overhead. Also, I noticed it doesn’t always work. I tried deleting a contact without the trailing slash and APPEND_SLASH gave an error as it wasn’t able to redirect it.
    {noformat}RuntimeError: You called this URL via DELETE, but the URL doesn't end in a slash and you have APPEND_SLASH set. Django can't redirect to the slash URL while maintaining DELETE data. Change your form to point to localhost:8080/api/v1/contacts/665c6c35-29fe-4f6c-b390-96d318627f95/ (note the trailing slash), or set APPEND_SLASH=False in your Django settings.{noformat}

  • I see two possible solutions.
    # Turn off APPEND_SLASH in django and update all the front end end points to be properly formatted.
    # Risks:
    #
    Potential for missing endpoints.
    #* New endpoints need to be properly formatted or the call will fail.
    # Benefits:
    #* Forces developers to properly format their api calls during development.
    # Use an interceptor on the front end to handle automatically adding the trailing slash before the request is sent.
    # Risks:
    #
    While we don’t have any endpoints that don’t have trailing slashes currently, we might in the future, and this would prevent those links from working.
    #** Wanted to point out that the oidc links still works since those are handled as href links which interceptors do not intercept.
    # Benefits:
    #* No risk of missing endpoint fixes on the front end.
    #* Technically we don’t have to turn off APPEND_SLASH in django, but I would still recommend it. Especially if we would rather have failures over expensive redirects and would still catch anything that somehow slipped through the the interceptor.

The second option is a lot easier and less time consuming to implement and has lower risk now, whereas the first option will require a comprehensive sweep of the front end, so it’ll take a lot longer to implement but we could run into issues in the future if we choose to break the django default of always having a trailing slash.

Copy link
Author

Sasha Dresden commented: Wanted to move this to Code Review but needed something for the PR field so I put the PR I put in for [https://fecgov.atlassian.net/browse/FECFILE-1785|https://fecgov.atlassian.net/browse/FECFILE-1785|smart-link] which implements the front-end interceptor to show how a demo of Option 2. As mentioned in the write-up, Option 1 would require a lot more time investment.

Copy link
Author

Todd Lees commented: solution found and ticket created [https://fecgov.atlassian.net/browse/FECFILE-1877|https://fecgov.atlassian.net/browse/FECFILE-1877|smart-link]

Copy link
Author

Shelly Wise commented: No code change or QA review needed on this ticket per DEV.

Moved to Stage Ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant