improve sanitization of watchdog link/operations item

Since the text is meant to be a link it should be filtered with something that will preserve the allowed html and get rid of xss, something like filter_xss (see this cheat sheet about which function to use).

I like option A (document it) for Drupal 6/7 and either your option B or option C - use filter_xss - for Drupal 8.

We would need to move filter_xss() from common.inc to bootstrap.inc if we want to call it from watchdog().

See #1174496: WSOD: drupal_error_handler() assumes filter_xss() is defined

We filter on output, so we don't need to move filter_xss() for this.

I believe the patch in #4 is correct but it would be nice to get another +1 from someone in the security team. Marking it RTBC.

+1 from me. filter_xss() is the appropriate filtering in this case.

After committing to 8.x, needs to be moved to 7.x for adding docs as suggested in #1/#2

Why can't the patch be applied to 6 and 7 also?

@jbrown - it probably could. I think there's a debate about whether that's a change of functionality or not. This should be seemless for Drupal 6/7 (I doubt anyone is putting javascript in there) so it would make some sense to backport.

Agreed.

Committed to 8.x. Now we need to backport a solution to 7.x. See #2.

I thought the consensus was that the same patch could be applied to 6, 7 and 8.

Putting this to CNR for D7 since the patch applies cleanly and we need a consensus between a new patch and committing the existing one.

There are two options:

1. Apply the patch from #4 as is to Drupal 7, there is a chance that someone is doing something really fancy there that would break, but it's likely a theoretical problem.

2. Write a new patch that documents the issue for module maintainers, but doesn't add the filter_xss() at all.

I would personally go for #1 - would rather have the security hardening - it is just a link in the watchdog table.

Also this is dblog module which doesn't really expose an API - if you use syslog module it handles the links differently anyway, so modules can't rely 100% on the link being rendered as HTML in a table.

I I agree with catch in #15 - we should apply #4 as is to Drupal 7.

Since that's three of us, and greggles posted #2 which had the choices, I'm going to move this back to RTBC.

I think this makes sense, too. Worst-case, a table buried in an administrative page whose table rows get pruned every 1000 entries or so looks weird for a few minutes.

Committed to 7.x. Marking back for 6.x.

No longer needs backport to D7.

I don't understand the updated comment in #18. Was it meant for a different issue?

@jbrown - http://drupalcode.org/project/drupal.git/commit/2d884d2 The code was committed. I think the comment makes sense: she's saying this is a bit of an API change but that it's unlikely to affect people and, even if it does affect them, it just affects one part of a page that is not frequently used.

Okay - I understand webchick's comment now.

Anybody tested on Drupal 6?

#1235536: too long link breaks admin/report/dblog table related issue

Anybody tested on Drupal 6?

I have now, and can confirm it works as expected. Straight reroll of the patch in #4 since it no longer applied cleanly.

Thanks, committed.