https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37573
David Cook <dc...@prosentient.com.au> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|Failed QA |Signed Off
Text to go in the|This fixes the inability to |Restrict the removal of {}
release notes|use JavaScript inside of |tokens in
|<script> tags in |OPACSearchForTitleIn
|OPACSearchForTitleIn |preference to only
|preference (the code was |underscores and
|unexpectedly butchered, |alphanumeric to stop
|causing syntax errors) |unintended removal of other
| |data including {}
| |characters.
Summary|Bad escaping in |Restrict
|OPACSearchForTitleIn breaks |OPACSearchForTitleIn token
|JS in <script> tags |removal to underscore and
| |alphanumeric
--- Comment #12 from David Cook <dc...@prosentient.com.au> ---
(In reply to Michał from comment #11)
> I don’t think that’s right. If Koha was to limit where scripts are able to
> execute, it should do it universally and non-discriminatorily across all
> prefs and other places alike where HTML can be inserted.
It's a work in progress, but believe me when I say it's coming.
> With the way things
> are currently, broken greedy token parsing is by all means a bug and
> unexpected behavior. The coincidental alignment of its practical end effect
> with one of long-term goals of Koha development shouldn’t stop its fixing…
> It shouldn’t willingly be kept broken for this reason.
I think there's some merit to what you're saying there. In fact, I think that
there's an argument to be made for removing the removal of unsupported tokens
completely, since it's not really necessary. (We could add the supported tokens
to the syspref description and then any unsupported tokens are the library's
responsibility to remove.)
But your change here would also stop many potential tokens from being removed,
which some could say is a regression of the current behaviour.
> JavaScript is not the only thing someone could have there. Imagine someone
> could want to put a HTML attribute like data-json='{"something":1}' for
> example. It’s not JavaScript, and props like that are used sometimes, it’s
> another “safe” use-case that’s being broken.
I'd advise against that coding as well. You'd be better off with
data-something="1" "data-somethingelse="2".
Plus that's a pretty strange use-case for OPACSearchForTitleIn. Keep in mind
too bug 36805 which would proably also make that impossible.
> I mean having them adhere to some common and very specific format unlikely
> to conflict with random things is infinitely better than parsing that
> matches anything in between {} and thus breaks stuff.
I think that's debateable, but tell you what... I'll reset this to "Signed
Off". I am going to change the title and release notes to reflect the actual
change made.
> > I'm going to open a new bug report which will specifically prevent the
> > injection of Javascript via this system preference.
>
> I think one should be opened for all HTML system prefs and things like this
> alike, I don’t see a reason to single out this one specifically (I think
> there are more, right?).
There is one already for all things, but it's huge and very slow moving.
There's a saying in English "the squeaky wheel gets the oil". This preference
has become very noticeable (you've specifically mentioned injecting Javascript
in it, for instance), and it's typically limited in scope ( as noted on bug
36805 it's typically a list of links ), so theoretically easier to restrict
than other sysprefs.
> Also not sure if this blockage can really be done
> properly without using nonces. Naive breaking such as removing {} does not
> currently prevent JS code without code blocks from running at all. Matching
> <script> tags also has a lot of caveats. I’m not sure if any temporary
> solution other than the said eventual planned script execution source
> limiting via CSP would make much sense.
We frequently use HTML::Scrubber via C4::Scrubber in a number of places to
sanitize HTML outputs. It can be very effective. Brutal at times but effective.
And yes CSP is coming too, so as per above... use OpacUserJS for injecting
Javascript. Otherwise, you're just going to have to change your code later. May
as well future-proof yourself a bit now.
--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
