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.

Sign up for GitHub

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

Jump to bottom

& vs & in e_url addon and validation #4054

Closed
Jimmi08 opened this issue Dec 30, 2019 · 8 comments · Fixed by #4067
Closed

& vs & in e_url addon and validation #4054

Jimmi08 opened this issue Dec 30, 2019 · 8 comments · Fixed by #4067
Assignees
@Deltik
Labels
type: bug A problem that should not be happening
Milestone
e107 2.3.0

Comments

@Jimmi08
Copy link
Contributor

Jimmi08 commented Dec 30, 2019

Bug Description

e_url uses & in URL configuration and it causes validation error with HTML validator extension

How to Reproduce

Steps to reproduce the behavior:

  1. I noticed this with the forum plugin or use newforumposts menu

Expected Behavior

No warning error, but W3C validation didn't find this, so maybe enough is just to explain why did you decide to use & in httpBuildQuery method instead of &. Thanks

Screenshots

image
@Jimmi08 Jimmi08 added the type: bug A problem that should not be happening label Dec 30, 2019
@Jimmi08
Copy link
Contributor Author

Jimmi08 commented Dec 30, 2019

@Moc it should be labeled as a question, it wasn't intention marked it as bug

@Deltik
Copy link
Member

Deltik commented Dec 30, 2019 •
edited
Loading

An HTML validation error caused by e107 sounds like a bug to me. I'll add an issue template for the question label.

Update: Issue template for questions is live!

@Jimmi08
Copy link
Contributor Author

Jimmi08 commented Jan 7, 2020

@Deltik Great. Thank you.
It's easier this way. I still dislike to mark something like bug, or enhancement, it should be on core developers to mark this way. I just noticed this type of labeling in other repository (that word type):

image

Deltik added a commit that referenced this issue Jan 7, 2020
@Deltik
Categorized issue types under "types:"
08b9041 
Based on feedback from #4054 (comment)
@Deltik
Copy link
Member

Deltik commented Jan 7, 2020

@Jimmi08: Thanks for the feedback! I've prepended all the issue type labels with "type: ".

As for the auto-type assignment, the types are fundamentally different, so it makes sense to add them right off the bat. If that's a mistake for whatever reason, a maintainer can change it later.

Issue statuses are now prepended with "status: ", so that should make statuses clearer, too.

@Jimmi08
Copy link
Contributor Author

Jimmi08 commented Jan 7, 2020

@Jimmi08 Perfect! Thank you.

@Deltik Deltik self-assigned this Jan 12, 2020
@Deltik Deltik added the status: pending This issue is being worked on or is in the backlog to be fixed. label Jan 12, 2020
@Deltik Deltik mentioned this issue Jan 12, 2020
Merged
10 tasks
Deltik added a commit to Deltik/e107 that referenced this issue Jan 12, 2020
@Deltik
Old HTML ampersand strict compatibility
e62422d 
- FIX: e107::url() now puts "&" in the query string instead of "&" for compliance with the older,
       looser definition of ambiguous ampersands in the HTML specification.
       Fixes: e107inc#4054
- FIX: Typo in comment
- FIX: Clear the core/e107/addons/e_url registry (cache) because if a plugin is installed after that
       cache is initialized, the cache is not updated anymore. The plugin's e_url is therefore not
       loaded, so SEF URLs won't be generated for that plugin until the cache is regenerated.
- NEW: Test for e107inc#4054
- FIX: e_pluginTest::testGetFields() expects the initial condition of the "forum" plugin to be
       uninstalled.
@Deltik
Copy link
Member

Deltik commented Jan 13, 2020

@Jimmi08: #4067 should fix this issue. Can you try to apply the following patch (same changes as #4067) to see if the issue is fixed?: fix-4054.patch

@Deltik Deltik added status: awaiting feedback This issue may be fixed and is awaiting the original poster to confirm the fix. and removed status: pending This issue is being worked on or is in the backlog to be fixed. labels Jan 13, 2020
@Jimmi08
Copy link
Contributor Author

Jimmi08 commented Jan 14, 2020

@Deltik Thanks. I will do it ASAP.

Could this be related?: #3943
the point is that if there is & then that test fails because of ';' in the query.
I have no idea why it just doesn't work in some cases.

@Jimmi08
Copy link
Contributor Author

Jimmi08 commented Jan 14, 2020

What I did (I never used patch file before):
I am using this addon for testing:
http://users.skynet.be/mgueury/mozilla/

  • sync with Github via admin
  • on local repository applied your patch txt file, only e107_class.php was changed
  • manual upload this file to the server
  • cleared all caches (this was important)

Result:

  • homepage (newsforum menu is there) - all similar errors fixed
  • forum plugin - checked all pages (viewforum, viewtopic etc) - all similar errors fixed,
  • checked download, news, gallery.

@Deltik Deltik removed the status: awaiting feedback This issue may be fixed and is awaiting the original poster to confirm the fix. label Jan 14, 2020
@CaMer0n CaMer0n reopened this Jan 14, 2020
@CaMer0n CaMer0n closed this as completed Jan 14, 2020
Deltik added a commit that referenced this issue Jan 14, 2020
@Deltik
Prevent HTML code injection in e107::url()
34047a2 
Fixes: #4054

This is a better fix for #4054. HTML code injection can no longer occur in URLs generated by
e107::url() thanks to htmlspecialchars(). The previous implementation only addressed:

    & => &

Now, quotation marks and alligator brackets are also escaped, so:

    <a href=""></html>"></a>

is now rendered as:

    <a href="&quot;&gt;&lt;/html&gt;"></a>
@CaMer0n CaMer0n added this to the e107 2.3.0 milestone Jan 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Deltik Deltik

Labels
type: bug A problem that should not be happening
Projects
None yet
Milestone
e107 2.3.0
Development

Successfully merging a pull request may close this issue.

Compliant query string ampersands in e_url Deltik/e107
3 participants
@Deltik @CaMer0n @Jimmi08