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

[4.0] Move the additional language strings in a separate file & fix postinstall #19449

Merged
merged 39 commits into from Mar 9, 2018

Conversation

zero-24
Copy link
Contributor

@zero-24 zero-24 commented Jan 24, 2018

Pull Request for Issue #18301 (comment)

Summary of Changes

Is there a reason why all the language strings are in the .sys.ini language file and not spread across two language files as normal

Thanks @brianteeman

  • Fixing the postinstall behavior as this was broken before.

Testing Instructions

  • install this branch https://github.com/zero-24/joomla-cms/archive/httpheader.zip
  • Check the config page of the httpheader plugin and make sure all language strings are still loaded. -
  • Check the postinstall message and make sure all language strings are still loaded too. (please make sure to disable the plugin inorder to trigger the postinstall message)

Expected result

two files & postinstall works also on new installs

Actual result

one file & postinstall is broken.

@infograf768
Copy link
Member

The xml file has no manifest part for languages. I suggest to add them.

	<languages>
		<language tag="en-GB">en-GB.plg_system_httpheader.ini</language>
		<language tag="en-GB">en-GB.plg_system_httpheader.sys.ini</language>
	</languages>

@infograf768
Copy link
Member

There are other things that can be done for customhttpheader.xml but it could be done in another PR;

  1. windows eol should be modified to Unix
  2. No use to utilize specific strings for Site and Administrator as we already have the global ones, JSITE and JADMINISTRATOR.
  3. String PLG_SYSTEM_HTTPHEADER_ADDITIONAL_HEADER_CLIENT="Site selection" proposing Site, Administrator or Both, makes no sense. It should have "Client" as value.

@zero-24
Copy link
Contributor Author

zero-24 commented Jan 25, 2018

Thanks fixed.

$query = $db->getQuery(true)
->update($db->quoteName('#__extensions'))
->set($db->quoteName('params') . ' = ' . $db->quote($params))
->where($db->quoteName('extension_id') . ' = 487')
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be 700?

Copy link
Member

Choose a reason for hiding this comment

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

As far as I see 700 is a common id in postinstall table, 487 in the extensions table

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry it looks like i missed your messages :(

I have now choose a different way to solve this problem by renaming the core plugin it is not going to conflict with the 3.x version on a upgrade. So no need for that complete sql stuff.

@zero-24
Copy link
Contributor Author

zero-24 commented Feb 24, 2018

The Enable default security headers button does nothing.

can you please double check that? It should just enable the plugin currently there is no redirection implemented.

PLG_SYSTEM_HTTPHEADERS_ADDITIONAL_HEADER_KEY="HTTP Header"
PLG_SYSTEM_HTTPHEADERS_ADDITIONAL_HEADER_VALUE="HTTP Header Value"
PLG_SYSTEM_HTTPHEADERS_POSTINSTALL_INTRODUCTION_TITLE="HTTP Security Headers"
PLG_SYSTEM_HTTPHEADERS_POSTINSTALL_INTRODUCTION_BODY="<p>Joomla! comes with a built-in plugin that handles http security headers. It helps to secure your site by setting the following headers with the default values:<br><ul><li><a href='https://scotthelme.co.uk/hardening-your-http-response-headers/#x-frame-options'>'X-Frame-Options: SAMEORIGIN'</a></li><li><a href='https://scotthelme.co.uk/hardening-your-http-response-headers/#x-xss-protection'>'X-XSS-Protection: 1; mode=block'</a></li><li><a href='https://scotthelme.co.uk/hardening-your-http-response-headers/#x-content-type-options'>'X-Content-Type-Options: nosniff'</a></li><li><a href='https://scotthelme.co.uk/a-new-security-header-referrer-policy/'>'Referrer-Policy: no-referrer-when-downgrade'</a></li></ul><br>The full list of supported headers are: <br><ul><li><a href='https://en.wikipedia.org/wiki/HTTP_Strict_Transport_Security'>Strict-Transport-Security</a></li><li><a href='https://en.wikipedia.org/wiki/Content_Security_Policy'>Content-Security-Policy</a></li><li>Content-Security-Policy-Report-Only</li><li>X-Frame-Options</li><li>X-XSS-Protection</li><li>X-Content-Type-Options</li><li>Referrer-Policy</li><li>Expect-CT</li></ul><br>These headers help your browser to protect your website from <a href='https://en.wikipedia.org/wiki/Cross-site_scripting'>XSS</a> and <a href='https://en.wikipedia.org/wiki/Clickjacking'>Clickjacking</a> attacks.</p>"
Copy link
Contributor

Choose a reason for hiding this comment

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

<ul> tag should not be inside a <p> tag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

<p> - tag is removed now.

@Quy
Copy link
Contributor

Quy commented Feb 24, 2018

Still disabled. I expect the postinstall message to disappear after clicking the button if the plugin is to be enabled.

@zero-24
Copy link
Contributor Author

zero-24 commented Feb 24, 2018

Still disabled. I expect the postinstall message to disappear after clicking the button if the plugin is enabled.

Sure. hmm I'm going to debug this one. Maybe i'm also going to redirect the user to the edit page so they can configure the needed values in the plugin if they hit enable. Let's see what is the problem and how to fix it.

@zero-24
Copy link
Contributor Author

zero-24 commented Feb 24, 2018

I have just fixed the button. A redirect to the plugin edit page is not possible ;)

@brianteeman
Copy link
Contributor

Yes it is - i did it in the recaptcha v1 pr #19648 or i misunderstand

@zero-24
Copy link
Contributor Author

zero-24 commented Feb 24, 2018

I'm going to review that proposal. Thanks!

@zero-24
Copy link
Contributor Author

zero-24 commented Feb 24, 2018

hmm look like this is working. My test before that resulted into a "you are not allowed to use this link directly" error. Now this is fixed. So commit is in coming. Thanks @brianteeman

@Quy
Copy link
Contributor

Quy commented Mar 2, 2018

The default headers are not set. A var dump of $this->params->get('xframeoptions') results in string(1) "1" which is not the same type as ===1.

		// X-Frame-Options
		if ($this->params->get('xframeoptions', 1) === 1)
		{
			$this->app->setHeader('X-Frame-Options', 'SAMEORIGIN');
		}

@zero-24
Copy link
Contributor Author

zero-24 commented Mar 2, 2018

Thanks should be fixed by the last commit.

@Quy
Copy link
Contributor

Quy commented Mar 5, 2018

I had to save the plugin initially before it would be in effect. 1 should be a string as follows:

get('xframeoptions', '1')

@Quy
Copy link
Contributor

Quy commented Mar 5, 2018

I am testing with your branch rather than via PatchTester. This is the error I get when doing a fresh install. I know it is unrelated to your PR, but how to fix it?
0 Could not create session directory "/var/lib/php/session"

@zero-24
Copy link
Contributor Author

zero-24 commented Mar 5, 2018

Hmm i have no idea how the session things works nowdays in 4.0 can your user write to that path?

@wilsonge wilsonge merged commit b176349 into joomla:4.0-dev Mar 9, 2018
@wilsonge
Copy link
Contributor

wilsonge commented Mar 9, 2018

Thanks Tobias :)

@zero-24 zero-24 deleted the httpheader branch March 9, 2018 21:30
@zero-24
Copy link
Contributor Author

zero-24 commented Mar 9, 2018

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language Change This is for Translators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants