Page MenuHomeSoftware Heritage

add-forge-now: Display forge url as link in moderation view
ClosedPublic

Authored by ardumont on Thu, Aug 4, 10:15 AM.

Diff Detail

Repository
rDWAPPS Web applications
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Build has FAILED

Patch application report for D8178 (id=29528)

Rebasing onto 6f223dcb66...

Current branch diff-target is up to date.
Changes applied before test
commit e5e1ee7d9d99dd131230fff66cd47ee8fe5702f9
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Aug 4 10:15:02 2022 +0200

    add-forge-now: Display forge url as link

Link to build: https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/1948/
See console output for more information: https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/1948/console

Harbormaster returned this revision to the author for changes because remote builds failed.Thu, Aug 4, 10:17 AM
Harbormaster failed remote builds in B30595: Diff 29528!

Build has FAILED

Patch application report for D8178 (id=29530)

Could not rebase; Attempt merge onto 6f223dcb66...

Updating 6f223dcb..094502ac
Fast-forward
 .../src/bundles/add_forge/moderation-dashboard.js   |  6 ++++--
 assets/src/bundles/admin/deposit.js                 | 17 +----------------
 assets/src/utils/functions.js                       | 18 +++++++++++++++++-
 swh/web/tests/data.py                               | 21 +--------------------
 4 files changed, 23 insertions(+), 39 deletions(-)
Changes applied before test
commit 094502ac959427bb9f69ea7ea9eb4985c06c1180
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Aug 4 10:15:02 2022 +0200

    add-forge-now: Display forge url as link

commit 89954bdce3389091d4bf4feeb188e241fa44fcd4
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Aug 4 10:22:56 2022 +0200

    tests/data: Drop ctags indexer reference
    
    They no longer exist.
    
    This fixes build [1]
    
    Related to T4273
    
    [1] https://jenkins.softwareheritage.org/view/swh%20master%20(draft)/job/DWAPPS/job/tests/3265/console

Link to build: https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/1950/
See console output for more information: https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/1950/console

Harbormaster returned this revision to the author for changes because remote builds failed.Thu, Aug 4, 10:43 AM
Harbormaster failed remote builds in B30597: Diff 29530!

Build is green

Patch application report for D8178 (id=29536)

Could not rebase; Attempt merge onto 6f223dcb66...

Updating 6f223dcb..47edc667
Fast-forward
 .../src/bundles/add_forge/moderation-dashboard.js  |  6 +-
 assets/src/bundles/admin/deposit.js                | 17 +---
 assets/src/utils/functions.js                      | 18 ++++-
 swh/web/api/views/content.py                       | 64 ---------------
 swh/web/common/archive.py                          | 48 ------------
 swh/web/tests/api/views/test_content.py            | 90 +---------------------
 swh/web/tests/common/test_archive.py               | 61 +--------------
 swh/web/tests/common/test_utils.py                 |  1 -
 swh/web/tests/conftest.py                          | 46 -----------
 swh/web/tests/data.py                              | 21 +----
 10 files changed, 25 insertions(+), 347 deletions(-)
Changes applied before test
commit 47edc6679ea64f1420da3c8d6f8a419dbaaca811
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Aug 4 10:15:02 2022 +0200

    add-forge-now: Display forge url as link in moderation view

commit 3e5d552a328353fafa505904c664486c4ad2b05c
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Aug 4 10:22:56 2022 +0200

    Drop ctags indexer references and hidden api using it
    
    The ctags indexer has been stopped a long time ago and recently got dropped from the
    indexer base code. As the api exposing this in the webapp is hidden, we can safely
    remove them.
    
    This also fixes build [1].
    
    Related to T4273
    
    [1] https://jenkins.softwareheritage.org/view/swh%20master%20(draft)/job/DWAPPS/job/tests/3265/console

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/1954/ for more details.

vlorentz requested changes to this revision.Thu, Aug 4, 4:37 PM
vlorentz added a subscriber: vlorentz.
vlorentz added inline comments.
assets/src/bundles/add_forge/moderation-dashboard.js
51

you should sanitize it here because...

assets/src/utils/functions.js
172

encodeURI is not sufficient to sanitize; which means maliciously crafted URLs can be used to XSS; for example by submitting javascript:alert(42) as URL.

This revision now requires changes to proceed.Thu, Aug 4, 4:37 PM

Additionally, the existing genLink is incorrect, because it assumes a sanitized URL (using DataTables' HTML escaping), then URL-encodes it to be used both as href and text.

While this is safe, this can link and display incorrect URLs; because it should be URL-encoded *before* being HTML-escaped. But this is probably out of scope for this diff.

assets/src/bundles/add_forge/moderation-dashboard.js
51

i had that initially and then i even pushed it inside the genLink function...
but i grew tired of it (and since it's not well tested, i wanted to change as few things as possible)...
so i reverted everything ;)

assets/src/utils/functions.js
172

would having $.fn.dataTable.render.text().display(data); called within here be enough?
(it was one of my first iterations)

assets/src/utils/functions.js
172

Feels weird to use datatable here, but sure. Alternatively, you can use any of these methods: https://stackoverflow.com/questions/24816/escaping-html-strings-with-jquery

assets/src/utils/functions.js
172

also, I'm not sure it would try to escape " characters, $.fn.dataTable.render.text().display(data) is designed to escape HTML; not attribute values.

And in particular, I just realized we should whitelist schemes; because typical escaping won't filter out javascript: pseudo-URLs

Adapt according to review and unify one missed add-forge-now view as well with this

This revision is now accepted and ready to land.Thu, Aug 4, 5:24 PM

Build is green

Patch application report for D8178 (id=29560)

Rebasing onto 3e5d552a32...

Current branch diff-target is up to date.
Changes applied before test
commit 708fb873223aa79657d551a89b0a464988e1e1a4
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Aug 4 10:15:02 2022 +0200

    add-forge-now: Display forge url as link in moderation view

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/1955/ for more details.