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

Allow to rename api.php #2132

Merged
merged 6 commits into from
Jul 15, 2022
Merged

Allow to rename api.php #2132

merged 6 commits into from
Jul 15, 2022

Conversation

luigifab
Copy link
Contributor

@luigifab luigifab commented May 18, 2022

Description

This PR allow you to rename your api.php to something else to avoid kevin's attack on it.
Like you didn't use /admin for backend.

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)

@github-actions github-actions bot added Component: Api PageRelates to Mage_Api Component: Api2 Relates to Mage_Api2 labels May 18, 2022
@elidrissidev
Copy link
Member

to avoid kevin's attack on it.

What's kevin's attack? Can you provide a related link?

@luigifab
Copy link
Contributor Author

Yes, at my office, we have a problem with some "developers" that we call Kévin, that copy/paste stackoverflow, and who don't understand that they are doing. This change will prevent brute force attacks on API if you rename your file, because only you and your providers know API url.

@elidrissidev
Copy link
Member

elidrissidev commented May 20, 2022

Not only in your office, unfortunately. I literally googled that before asking thinking it was some known vulnerability 🤣.

@fballiano
Copy link
Contributor

should we document somewhere how to use a different url for /api?

@luigifab
Copy link
Contributor Author

luigifab commented Jun 8, 2022

Yes, in my to do list.

@ADDISON74
Copy link
Contributor

This appears in .htaccess

############################################
## rewrite API2 calls to api.php (by now it is REST only)

    RewriteRule ^api/rest api.php?type=rest [QSA,L]

If you rename the api.php file what do we do with the .htaccess setting?

@luigifab
Copy link
Contributor Author

Yes you do (at htaccess level, or web server config level).

Copy link
Contributor

@fballiano fballiano left a comment

Choose a reason for hiding this comment

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

There's a typo: "Don't forgot to also update server configuration" should be " Don't forget to also update server configuration"

@ADDISON74
Copy link
Contributor

ADDISON74 commented Jul 12, 2022

I propose the following message:

Don't use common file names like api.php for OpenMage API URLs to prevent attacks. Don't use the new file name in _robots.txt_ and keep it secret with your partners. After renaming the file you must update the webserver configuration as follows

* Apache .htaccess: `RewriteRule ^api/rest api.php?type=rest [QSA,L]`
* Nginx: `rewrite ^/api/(\w+).*$ /api.php?type=$1 last;`

fballiano
fballiano previously approved these changes Jul 15, 2022
@ADDISON74
Copy link
Contributor

Thank you for taking my suggestion into account, but you had to make the change in the other paragraph as well to keep the same addressing mode. I allowed myself to do it.

I personally have no other comments. We can merge this PR.

@ADDISON74
Copy link
Contributor

@elidrissidev - I only commented on the existing paragraphs. If new changes are needed please make a proposal and we implement it immediately. This PR allows us editing.

@elidrissidev
Copy link
Member

Sorry deleted my last comment by accident. This is what it said:

It should be noted that this change only affects the REST API as SOAP/Xmlrpc APIs are accessed directly from a controller (through index.php) and not through api.php. If it's not clear enough to everyone then it should be amended in the README.

@elidrissidev
Copy link
Member

elidrissidev commented Jul 15, 2022

Sorry deleted my last comment by accident. This is what it said:

It should be noted that this change only affects the REST API as SOAP/Xmlrpc APIs are accessed directly from a controller (through index.php) and not through api.php. If it's not clear enough to everyone then it should be amended in the README.

Actually I retract what I said 😅, it is indeed possible to use api.php as a secondary way to access SOAP/Xmlrpc APIs, it's just not possible by default. So with the below change in .htaccess you will be able to access all types of APIs through api.php from http://<magento_host>/api/<code>/, where code is either rest, soap, soap_v2, soap_wsi, or xmlrpc!

## uncomment next line to enable light API calls processing

-#    RewriteRule ^api/([a-z][0-9a-z_]+)/?$ api.php?type=$1 [QSA,L]
+    RewriteRule ^api/([a-z][0-9a-z_]+)/?$ api.php?type=$1 [QSA,L]

############################################
## rewrite API2 calls to api.php (by now it is REST only)

-    RewriteRule ^api/rest api.php?type=rest [QSA,L]
+#    RewriteRule ^api/rest api.php?type=rest [QSA,L]

So this PR looks good to me.

@fballiano fballiano merged commit 0d99717 into OpenMage:1.9.4.x Jul 15, 2022
@github-actions
Copy link
Contributor

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
7 runs  ±0  5 ✔️ ±0  2 💤 ±0  0 ❌ ±0 

Results for commit 0d99717. ± Comparison against base commit f0c14a0.

@elidrissidev
Copy link
Member

Another discovery I made is that updating the filename alone is not enough because you'll get "Request does not match type route." 404 error with the REST Api. To fix this you'll also need to update the URL in .htaccess:

############################################
## rewrite API2 calls to api.php (by now it is REST only)

-    RewriteRule ^api/rest api.php?type=rest [QSA,L]
+    RewriteRule ^ipa/rest ipa.php?type=rest [QSA,L]

@ADDISON74
Copy link
Contributor

ADDISON74 commented Jul 15, 2022

In the OpenMage documentation the author mentioned the following:

Don't use common file names like api.php for OpenMage API URLs to prevent attacks. Don't use the new file name in _robots.txt_ and keep it secret with your partners. After renaming the file you must update the webserver configuration as follows:

* Apache .htaccess: `RewriteRule ^api/rest api.php?type=rest [QSA,L]`

He practically requested a mandatory change for the two webservers.

@luigifab luigifab deleted the rename-api branch July 15, 2022 15:46
elidrissidev pushed a commit to elidrissidev/magento-lts that referenced this pull request Jul 22, 2022
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.

5 participants