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

Accessibility and security issues [BREAKING CHANGE] #76

Merged
merged 15 commits into from
Dec 27, 2022
Merged

Accessibility and security issues [BREAKING CHANGE] #76

merged 15 commits into from
Dec 27, 2022

Conversation

flashadvocate
Copy link
Collaborator

@flashadvocate flashadvocate commented Dec 27, 2022

Takes care of a few accessibility issues mentioned in #59 though there's plenty of room for additional work resolving the 190 missing alt attributes across the application's images.

Also makes a breaking change that needs to be incorporated in existing installations of minimanager.

Somewhere, preferably at the bottom of the config.php, the following should be added:

if (!defined('HEADER_LOADED')) {
  header('Location: /');
}

Right now, due to the nature of the application, it is possible to navigate directly to the config file. While this isn't an issue in an ideal world, it does a couple things

  • confirms for the end user that config.php exists at that location
  • creates the possibility of leaked credentials in a misconfigured state

This is also the reason for dropping index.html files in favor of server-side redirects, as these too paint a vivid picture of the directory structure.

Ideally, apache/nginx should forward all requests to a front controller (index.php) and handle routing at the application layer. Navigating directly to individual PHP files makes it much easier for attackers to glean potentially useful information.

An alternative would be to move the config.php file somewhere outside the server's docroot to prevent navigating to the file at all. Neither are ideal or elegant, but in lieu of a complete overhaul (which is coming), this provides some additional protection.

and titles to hidden submit buttons. Also omit empty values from submit buttons.
Dropping these in favor of server-side redirects. Providing a "oops, you aren't supposed to be here" simply exposes directory structure to the end user.

Ideally, apache/nginx should forward all requests to a front controller (index.php) and handle routing at the application layer. Navigating directly to individual PHP files makes it hard to obscure details about the application.
Though it is PHP and thus will render nothing in the browser, we don't want to expose its existence.

There's also a possibility that, given a broken or misconfigured state, this file could output its contents
@flashadvocate flashadvocate changed the title Accessibility security issues Accessibility and security issues Dec 27, 2022
@flashadvocate flashadvocate changed the title Accessibility and security issues Accessibility and security issues [BREAKING CHANGE] Dec 27, 2022
@flashadvocate flashadvocate marked this pull request as ready for review December 27, 2022 02:54
@Faq
Copy link

Faq commented Dec 27, 2022

adding a lot of header('Location: /'); doesnt looks like DRY

@flashadvocate
Copy link
Collaborator Author

flashadvocate commented Dec 27, 2022

adding a lot of header('Location: /'); doesnt looks like DRY

Nothing about this project is DRY 😂

Also note that the code replaced was the same thing, only an html redirect. The goal is to harden the application with this particular PR.

I’m working on a port of this project to laravel, so this is just a means to an end.

@flashadvocate
Copy link
Collaborator Author

@Faq hopefully I didn't come off as snarky. I do not like the methods at present, and I'm working to try and update some of the crazy. Baby steps, because there's a lot of crazy :)

@Aokromes Aokromes merged commit 4502b34 into TrinityCore:master Dec 27, 2022
@flashadvocate flashadvocate deleted the accessibility_security_issues branch December 27, 2022 12:54
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

Successfully merging this pull request may close these issues.

3 participants