Skip to content

Conversation

kiatng
Copy link
Contributor

@kiatng kiatng commented Oct 10, 2024

Description (*)

Ref #1209 (comment), where I was trying to make use of the nginx config for admin without the use of caddy. For this, I needed the ability to have a custom admin URL, which is configurable here:

image

However, Custom Admin URL is not implemented. This PR is my attempt to complete the implementation.

When Custom Admin URL is used, frontend access to admin is forbidden in the function match() in app/code/core/Mage/Core/Controller/Varien/Router/Admin.php. This is independent of server config, which provides an alternate way:

# Non-rewritten URLs, Admin and API are disabled for frontend
location /index.php/ { return 404; }
location ~ ^/admin(?:/(.*))?$ { return 404; }
location /api/ { return 404; }
location /api.php { return 404; }

Related Pull Requests

PR #1209

Manual testing scenarios (*)

  1. Add a separate host for admin, example nginx config:
server {
    listen 80;
    server_name admin.example.com;

    access_log /var/log/nginx/admin.example.com-access.log combined;
    error_log /var/log/nginx/admin.example.com-error.log;

    set $webroot /var/web/example; # OpenMage root

    include include/openmage-admin.conf; # See dev/openmage/nginx-admin.conf
}

For testing in DDEV, see this comment.
2. Go to admin > System > Configuration > Advanced / Admin > Admin Base URL
Set the Custom Admin URL and save:
image

If skip the above step in admin panel, you can update the table core_config_data directly:
Do not update the table directly, use the admin panel to configure the custom admin URL. The following is only for reference.

 -- UPDATE `core_config_data` SET `value` = '1' WHERE `path` = 'admin/url/use_custom';
INSERT INTO `core_config_data` (`scope`, `scope_id`, `path`, `value`, `updated_at`) 
  VALUES ('default', '0', 'admin/url/use_custom', '1', CURRENT_TIMESTAMP);
INSERT INTO `core_config_data` (`scope`, `scope_id`, `path`, `value`, `updated_at`) 
  VALUES ('default', '0', 'admin/url/custom', 'admin.example.com', CURRENT_TIMESTAMP);
INSERT INTO `core_config_data`  (`scope`, `scope_id`, `path`, `value`, `updated_at`) 
    VALUES ('stores', '0', 'web/secure/base_url', 'admin.example.com', CURRENT_TIMESTAMP);
INSERT INTO `core_config_data`  (`scope`, `scope_id`, `path`, `value`, `updated_at`) 
    VALUES ('stores', '0', 'web/unsecure/base_url', 'admin.example.com', CURRENT_TIMESTAMP);
image
  1. On saving the config, you will be redirected to the custom admin URL, login as usual. You can test again by disabling the custom URL, once saved, the admin URL is back to before.
  2. Navigate to main store www.example.com/adminFrontName should return 404 page not found.

Questions or comments

I am not sure if this is the best way to implement the custom admin URL. Collab welcome. With AI help, I think the implementation now is good.

Possible issue for API in 3rd-party code because of

Mage::init('admin');

As can be seen above, API is admin store, and if there is a Mage::getUrl() in the response params, the URL will not be accessible. There is no Mage::getUrl() in the API core, but may be present in 3rd-party code/modules.

This PR does not test for the custom path, the suffix in the admin URL.

@github-actions github-actions bot added Component: Core Relates to Mage_Core Component: Adminhtml Relates to Mage_Adminhtml labels Oct 10, 2024
@kiatng kiatng marked this pull request as draft October 10, 2024 03:56
@sreichel
Copy link
Contributor

I did a quick test ...

It seems it needs some documentation. Changes to .htaccess etc ...

I tried with apache (DDEV), added addtional host names, but got redirected to frontend page.

@github-actions github-actions bot added the errors Relates to error pages label Oct 11, 2024
@kiatng kiatng marked this pull request as ready for review January 20, 2025 04:31
@sreichel
Copy link
Contributor

sreichel commented Jan 22, 2025

Can you please combine the if statements as suggested?

@kiatng
Copy link
Contributor Author

kiatng commented Feb 13, 2025

This code is in production.

Copy link

Copy link

@sreichel
Copy link
Contributor

@kiatng cant login to admin with that change.

@kiatng kiatng requested a review from Copilot September 25, 2025 05:10
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements custom admin URL functionality to enhance security by allowing admin interfaces to be accessed via a separate host/domain. The feature enables blocking frontend access to the admin interface when a custom admin URL is configured.

  • Adds logic to check and enforce custom admin URL restrictions
  • Implements URL replacement for admin stores when custom admin URL is configured
  • Fixes an unrelated issue with error directory path validation

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
errors/processor.php Fixes path reference for error directory validation
app/code/core/Mage/Core/Model/Store.php Replaces base URLs with custom admin URL for admin stores
app/code/core/Mage/Core/Controller/Varien/Router/Admin.php Blocks admin access when request doesn't match custom admin URL
app/code/core/Mage/Adminhtml/Helper/Data.php Adds helper method to retrieve custom admin URL configuration

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@addison74 addison74 requested a review from Copilot September 27, 2025 22:44
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@kiatng
Copy link
Contributor Author

kiatng commented Oct 5, 2025

@sreichel To test in ddev:

.ddev/config.yaml:

additional_hostnames:
    - admin

The admin URL is: https://admin.ddev.site

Configure it with:

DELETE FROM core_config_data WHERE path IN ('admin/url/use_custom', 'admin/url/custom');
   INSERT INTO core_config_data (scope, scope_id, path, value) VALUES
   ('default', 0, 'admin/url/use_custom', '1'),
   ('default', 0, 'admin/url/custom', 'https://admin.ddev.site');

AI has opinions on my implementation, I will investigate them, so I put this on draft first.

@kiatng kiatng marked this pull request as draft October 5, 2025 09:12
@github-actions github-actions bot added the translations Relates to app/locale label Oct 7, 2025
kiatng and others added 4 commits October 7, 2025 15:19
AI is correct! It is bad that str_contains() exposes a security vulnerability, "https://example.com/admin" would be able to access the admin panel because the host "example.com" may be contains in the admin URL "https://admin.example.com/".

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@kiatng kiatng marked this pull request as ready for review October 8, 2025 12:12
@kiatng
Copy link
Contributor Author

kiatng commented Oct 8, 2025

I am quite happy about this PR, it's nice to increase security up a notch. Please test in real server as well as in DDEV.

Copy link

Please retry analysis of this Pull-Request directly on SonarQube Cloud

Copy link
Contributor

@sreichel sreichel left a comment

Choose a reason for hiding this comment

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

Works.

@addison74 addison74 merged commit e4d2290 into OpenMage:main Oct 13, 2025
21 checks passed
@sreichel sreichel deleted the custom_admin_url branch October 13, 2025 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component: Adminhtml Relates to Mage_Adminhtml Component: Core Relates to Mage_Core errors Relates to error pages new feature security translations Relates to app/locale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants