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

Do not set up PUI routes for handling slug-based URLs if use_human_readable_urls is false #3043

Merged

Conversation

andrew-morrison
Copy link
Contributor

Description

Similar to #2436, this pull request prevents the PUI from attempting to handle requests for slug-based URLs when AppConfig[:use_human_readable_urls] is false (which is the default value.) At present, whenever it receives a request that matches any of the slug-based URLs defined in routes.rb, it sends a request to the backend, which does a lookup in the slug column of the relevant database table. The PUI won't generate any slug-based links, but this route in particular...

get ":obj_type/:slug_or_id" => 'objects#show'

...means any URL like https://test.archivesspace.org/foo/bar will trigger a lookup. Such a URL might be something a plug-in developer wants to write a controller to handle, but cannot because the above route will take precedence. Also, similar URLs may be probed by bad actors looking for known vulnerabilities in other platforms, such as https://test.archivesspace.org/admin/login.php. They won't find a vulnerability, but it does trigger an unnecessary database lookup every time.

Related JIRA Ticket or GitHub Issue

None

How Has This Been Tested?

It does not affect existing routes using :rid and :id parameters, and obviously won't make any difference to organizations that do have human-readable URLs enabled. There are a few routes that do double duty for both slug-based and ID-based URLs, such as...

get "repositories/:slug_or_id" => 'repositories#show'
get "subjects/:slug_or_id" => 'subjects#show'

...so I have added another test for AppConfig[:use_human_readable_urls] in application_controller.rb to prevent those triggering unnecessary database lookups either.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have read the CONTRIBUTING document.
  • I have authority to submit this code.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@donaldjosephsmith
Copy link
Collaborator

@andrew-morrison looks like there are some rubocop violations that need attention before we can merge this.

@andrew-morrison
Copy link
Contributor Author

The rubocop violations have been fixed.

@cdibella cdibella added the community code contributed by community members not on or contracted by the ArchivesSpace program team label Oct 4, 2023
@brianzelip brianzelip self-assigned this Oct 25, 2023
@brianzelip
Copy link
Collaborator

Hi @andrew-morrison. We've got a suspicion the Classification Terms routes don't work as expected. We need to set something up to test but haven't gotten to it yet. It's on our radar and we'll follow up in the near future.

@brianzelip brianzelip merged commit b105859 into archivesspace:master Nov 17, 2023
8 checks passed
@cdibella cdibella added this to the 3.5.0 milestone Feb 7, 2024
thimios pushed a commit that referenced this pull request Apr 4, 2024
…adable_urls is false (#3043)

Prevent database lookups for slug URLs that remain if use_human_readable_urls is false
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community code contributed by community members not on or contracted by the ArchivesSpace program team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants