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

Removing root path from the entire output #9280

Closed
wants to merge 11 commits into from

Conversation

Radek-Suski
Copy link
Contributor

Pull Request for Issue #9252

Summary of Changes

Removing root path from the entire output

Testing Instructions

Check if the report contains the path to Joomla! installation

@@ -39,7 +39,7 @@ public function display($tpl = null)

$data = $this->getLayoutData();

echo json_encode($data);
echo str_replace(JPATH_ROOT, '', json_encode($data));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better as
echo str_replace(JPATH_ROOT, 'xxxxxx', json_encode($data));

So that it is clear there is something hidden in the output

@brianteeman
Copy link
Contributor

@Radek-Suski can you update this as per my comments so it is clear something has been removed from the output

Ignore the Travis failures they are unrelated

@Radek-Suski
Copy link
Contributor Author

Done

@brianteeman
Copy link
Contributor

Works fine for plain text

Maybe my suggested code was wrong for json as the json still shows the full path
eg

{"writable":false,"message":""},"cache":{"writable":true,"message":"COM_ADMIN_CACHE_DIRECTORY"},"administrator/cache":{"writable":true,"message":"COM_ADMIN_CACHE_DIRECTORY"},"/Applications/MAMP/htdocs/joomla-cms-staging/logs":{"writable":true,"message":"COM_ADMIN_LOG_DIRECTORY"},"/Applications/MAMP/htdocs/joomla-cms-staging/tmp":{"writable":true,"message":"COM_ADMIN_TEMP_DIRECTORY"}},"phpInfo":

@Radek-Suski
Copy link
Contributor Author

No, I missed a point. Give me few hours. I need to go out for a moment

@wilsonge
Copy link
Contributor

wilsonge commented Mar 2, 2016

Bump Radek :)

@Radek-Suski
Copy link
Contributor Author

@wilsonge I fixed it already

@andrepereiradasilva
Copy link
Contributor

@Radek-Suski one full path remains in the json: php open_basedir (if configured on the server), in txt is ok xxxxxxxx but in json is there

And there is also the /etc paths (openssl default config path, opcache blacklist path, for instance).
Don't know if they are a part of this PR though.

@brianteeman
Copy link
Contributor

@Radek-Suski Removing it completely (web and download) is not a good solution as it is useful for people to know this


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9280.

@brianteeman
Copy link
Contributor

I have tested this item 🔴 unsuccessfully on 1085f72


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9280.

@N6REJ
Copy link
Contributor

N6REJ commented Mar 3, 2016

What impact will this have on extensions that are looking for a file and
need the full path? Just continue as you normally do?
Bear

On 3/2/2016 17:53, Brian Teeman wrote:

@Radek-Suski https://github.com/Radek-Suski Removing it completely
(web and download) is not a good solution as it is useful for people
to know this


_This comment was created with the J!Tracker Application
https://github.com/joomla/jissues at
issues.joomla.org/joomla-cms/9280
https://issues.joomla.org/tracker/joomla-cms/9280.


Reply to this email directly or view it on GitHub
#9280 (comment).

@brianteeman
Copy link
Contributor

None at all
On 3 Mar 2016 1:02 am, "Bear" notifications@github.com wrote:

What impact will this have on extensions that are looking for a file and
need the full path? Just continue as you normally do?
Bear

On 3/2/2016 17:53, Brian Teeman wrote:

@Radek-Suski https://github.com/Radek-Suski Removing it completely
(web and download) is not a good solution as it is useful for people
to know this


_This comment was created with the J!Tracker Application
https://github.com/joomla/jissues at
issues.joomla.org/joomla-cms/9280
https://issues.joomla.org/tracker/joomla-cms/9280.


Reply to this email directly or view it on GitHub
#9280 (comment).


Reply to this email directly or view it on GitHub
#9280 (comment).

@Radek-Suski
Copy link
Contributor Author

@brianteeman we are removing it from all other paths so it would it doesn't make sense to make an exception for two paths only.
open_basedir would be hard to remove because it is not part of the Joomla! configuration.
I tested the solution and it worked for me. What doesn't work for you @brianteeman?

@brianteeman
Copy link
Contributor

@Radek-Suski We are ONLY removing the full path form the text and the json NOT the web
Example is Session Save Path which is displayed in full in the web interface but is obfuscated to session.save_path: xxxxxx in the json and text download

This last update from you removes the full path for logs and tmp folder in the folder permissions tab from the text, json AND web views it should not be removed from the web view.

Hope that makes sense


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9280.

@brianteeman
Copy link
Contributor

@andrepereiradasilva
I tested open_basedir and it is crrectly being removed from the text and json if it is set and still present in the web interface
https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_admin/models/sysinfo.php#L113

Not sure what @radek_suski meant about it not being something we can remove - he must need more coffee - as he wrote the code already


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9280.

@Radek-Suski
Copy link
Contributor Author

Ok, now I am a bit confused. What is a "web view"?

@brianteeman
Copy link
Contributor

The bit you can see in your web browser
On 3 Mar 2016 9:33 am, "Radek Suski" notifications@github.com wrote:

Ok, now I am a bit confused. What is a "web view"?


Reply to this email directly or view it on GitHub
#9280 (comment).

@joomla-cms-bot
Copy link

This PR has received new commits.

CC: @brianteeman


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9280.

@joomla-cms-bot
Copy link

This PR has received new commits.

CC: @brianteeman


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9280.

@brianteeman
Copy link
Contributor

Lets try testing it as well ;)

The txt and the json have

Warning: strstr() expects parameter 1 to be string, array given in /Applications/MAMP/htdocs/joomla-cms-staging/administrator/components/com_admin/models/sysinfo.php on line 186


The web now just says

log (Log folder) Writable
tmp (Temp folder) Writable

The web should be as it was before

/Applications/MAMP/htdocs/joomla-cms-staging/logs (Log folder) Writable
/Applications/MAMP/htdocs/joomla-cms-staging/tmp (Temp folder) Writable


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9280.

@richard67
Copy link
Member

Even worse now as before, as far as I could see log and temp dirs are also back with full path in json.

@joomla-cms-bot
Copy link

This PR has received new commits.

CC: @1apweb, @andrepereiradasilva, @anibalsanchez, @brianteeman, @richard67


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9280.

@andrepereiradasilva
Copy link
Contributor

I have tested this item ✅ successfully on 4d4e933

for me is ok
i would advice removing /etc paths too, but in another PR


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9280.

@richard67
Copy link
Member

I have tested this item ✅ successfully on 4d4e933

Tested with success, now both text and json format export files do not contain abolute paths anymore, also not for open_basedir, but in the backend sytem information views (html views) they still are shown as it should be.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9280.

@joomla-cms-bot
Copy link

This PR has received new commits.

CC: @1apweb, @andrepereiradasilva, @anibalsanchez, @brianteeman, @richard67


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9280.

@anibalsanchez
Copy link
Contributor

I have tested this item ✅ successfully on 60c327c

Test Ok. No paths.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9280.

@richard67
Copy link
Member

@Radek-Suski Shall we wait for further CS fixes for Travis? Or shall we test now? Let us know pls.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9280.

@Radek-Suski
Copy link
Contributor Author

Feel free to test. I have no time at the moment to pleasure Travis ;)

@richard67
Copy link
Member

OK. Was not your CS mistakes anyway.

@wilsonge
Copy link
Contributor

wilsonge commented Mar 3, 2016

It's fine - I can fix travis when i merge :)

@Radek-Suski
Copy link
Contributor Author

Thanks @wilsonge. You've a beer in Barcelona 💃

@andrepereiradasilva
Copy link
Contributor

it's already tested successfully several times, so can be merged IMHO

@wilsonge wilsonge added the RTC This Pull Request is Ready To Commit label Mar 3, 2016
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Mar 3, 2016
@wilsonge
Copy link
Contributor

wilsonge commented Mar 3, 2016

RTC

@wilsonge
Copy link
Contributor

wilsonge commented Mar 3, 2016

Will merge when I get home tonight.

@wilsonge wilsonge added the RTC This Pull Request is Ready To Commit label Mar 3, 2016
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Mar 3, 2016
@richard67
Copy link
Member

I have tested this item ✅ successfully on 60c327c


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9280.

@richard67
Copy link
Member

@wilsonge It seems joomla-cms-bot does not like you 👅 Maybe it's better with 2 good tests? Just tested with success.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9280.

@1apweb
Copy link

1apweb commented Mar 3, 2016

I have tested this item ✅ successfully on 60c327c


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9280.

@wilsonge wilsonge added the RTC This Pull Request is Ready To Commit label Mar 3, 2016
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Mar 3, 2016
@brianteeman
Copy link
Contributor

RTC (again)


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9280.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Mar 3, 2016
@wilsonge wilsonge closed this in 8159473 Mar 3, 2016
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Mar 3, 2016
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.

None yet

9 participants