diff --git a/assets/src/bundles/add_forge/create-request.js b/assets/src/bundles/add_forge/create-request.js --- a/assets/src/bundles/add_forge/create-request.js +++ b/assets/src/bundles/add_forge/create-request.js @@ -5,10 +5,12 @@ * See top-level LICENSE file for more information */ -import {handleFetchError, errorMessageFromResponse, csrfPost, - getHumanReadableDate, genLink} from 'utils/functions'; -import userRequestsFilterCheckboxFn from 'utils/requests-filter-checkbox.ejs'; import {swhSpinnerSrc} from 'utils/constants'; +import { + csrfPost, errorMessageFromResponse, genLink, getHumanReadableDate, + handleFetchError, validateUrl +} from 'utils/functions'; +import userRequestsFilterCheckboxFn from 'utils/requests-filter-checkbox.ejs'; let requestBrowseTable; @@ -122,3 +124,11 @@ ] }); } + +export function validateForgeUrl(input) { + let customValidity = ''; + if (!validateUrl(input.value.trim(), ['http', 'https'])) { + customValidity = 'The provided forge URL is not valid.'; + } + input.setCustomValidity(customValidity); +} diff --git a/assets/src/bundles/save/index.js b/assets/src/bundles/save/index.js --- a/assets/src/bundles/save/index.js +++ b/assets/src/bundles/save/index.js @@ -5,11 +5,13 @@ * See top-level LICENSE file for more information */ -import {csrfPost, handleFetchError, isGitRepoUrl, htmlAlert, - getCanonicalOriginURL, getHumanReadableDate} from 'utils/functions'; import {swhSpinnerSrc} from 'utils/constants'; -import artifactFormRowTemplate from './artifact-form-row.ejs'; +import { + csrfPost, getCanonicalOriginURL, getHumanReadableDate, handleFetchError, + htmlAlert, isGitRepoUrl, validateUrl +} from 'utils/functions'; import userRequestsFilterCheckboxFn from 'utils/requests-filter-checkbox.ejs'; +import artifactFormRowTemplate from './artifact-form-row.ejs'; let saveRequestsTable; @@ -352,21 +354,11 @@ export function validateSaveOriginUrl(input) { const originType = $('#swh-input-visit-type').val(); - let originUrl = null; - let validUrl = true; - - try { - originUrl = new URL(input.value.trim()); - } catch (TypeError) { - validUrl = false; - } + const allowedProtocols = ['http:', 'https:', 'svn:', 'git:', 'rsync:', + 'pserver:', 'ssh:', 'bzr:']; + const originUrl = validateUrl(input.value.trim(), allowedProtocols); - if (validUrl) { - const allowedProtocols = ['http:', 'https:', 'svn:', 'git:', 'rsync:', 'pserver:', 'ssh:', 'bzr:']; - validUrl = ( - allowedProtocols.find(protocol => protocol === originUrl.protocol) !== undefined - ); - } + let validUrl = originUrl !== null; if (validUrl && originType === 'git') { validUrl = isGitRepoUrl(originUrl); diff --git a/assets/src/utils/functions.js b/assets/src/utils/functions.js --- a/assets/src/utils/functions.js +++ b/assets/src/utils/functions.js @@ -96,17 +96,27 @@ return ``; } -export function isValidURL(string) { +export function validateUrl(url, allowedProtocols = []) { + let originUrl = null; + let validUrl = true; + try { - new URL(string); - } catch (_) { - return false; + originUrl = new URL(url); + } catch (TypeError) { + validUrl = false; } - return true; + + if (validUrl && allowedProtocols.length) { + validUrl = ( + allowedProtocols.find(protocol => protocol === originUrl.protocol) !== undefined + ); + } + + return validUrl ? originUrl : null; } export async function isArchivedOrigin(originPath) { - if (!isValidURL(originPath)) { + if (!validateUrl(originPath)) { // Not a valid URL, return immediately return false; } else { diff --git a/cypress/e2e/add-forge-now-request-create.cy.js b/cypress/e2e/add-forge-now-request-create.cy.js --- a/cypress/e2e/add-forge-now-request-create.cy.js +++ b/cypress/e2e/add-forge-now-request-create.cy.js @@ -43,7 +43,7 @@ cy.visit(this.addForgeNowUrl); // create requests for the user 'user' - populateForm('gitlab', 'gitlab.org', 'admin', 'admin@example.org', 'on', ''); + populateForm('gitlab', 'https://gitlab.org', 'admin', 'admin@example.org', 'on', ''); cy.get('#requestCreateForm').submit(); // user requests filter checkbox should be in the DOM @@ -62,9 +62,9 @@ cy.userLogin(); cy.visit(this.addForgeNowUrl); - populateForm('gitea', 'gitea.org', 'admin', 'admin@example.org', 'on', ''); + populateForm('gitea', 'https://gitea.org', 'admin', 'admin@example.org', 'on', ''); cy.get('#requestCreateForm').submit(); - populateForm('cgit', 'cgit.org', 'admin', 'admin@example.org', 'on', ''); + populateForm('cgit', 'https://cgit.org', 'admin', 'admin@example.org', 'on', ''); cy.get('#requestCreateForm').submit(); // user requests filter checkbox should be in the DOM @@ -208,7 +208,7 @@ it('should update browse list on successful submission', function() { cy.userLogin(); cy.visit(this.addForgeNowUrl); - populateForm('bitbucket', 'gitlab.com', 'test', 'test@example.com', 'on', 'test comment'); + populateForm('bitbucket', 'https://gitlab.com', 'test', 'test@example.com', 'on', 'test comment'); cy.get('#requestCreateForm').submit(); cy.visit(this.addForgeNowUrl); @@ -225,7 +225,7 @@ it('should show error message on conflict', function() { cy.userLogin(); cy.visit(this.addForgeNowUrl); - populateForm('bitbucket', 'gitlab.com', 'test', 'test@example.com', 'on', 'test comment'); + populateForm('bitbucket', 'https://gitlab.com', 'test', 'test@example.com', 'on', 'test comment'); cy.get('#requestCreateForm').submit(); cy.get('#requestCreateForm').submit(); // Submitting the same data again @@ -250,7 +250,7 @@ cy.visit(this.addForgeNowUrl); populateForm( - 'bitbucket', 'gitlab.com', 'test', 'test@example.com', 'off', 'comment' + 'bitbucket', 'https://gitlab.com', 'test', 'test@example.com', 'off', 'comment' ); cy.get('#requestCreateForm').submit(); @@ -261,4 +261,16 @@ }); }); + it('should bot validate form when forge URL is invalid', function() { + cy.userLogin(); + cy.visit(this.addForgeNowUrl); + populateForm('bitbucket', 'bitbucket.org', 'test', 'test@example.com', 'on', 'test comment'); + cy.get('#requestCreateForm').submit(); + + cy.get('#swh-input-forge-url') + .then(input => { + assert.isFalse(input[0].checkValidity()); + }); + }); + }); diff --git a/swh/web/add_forge_now/migrations/0008_turn_request_forge_url_into_url_field.py b/swh/web/add_forge_now/migrations/0008_turn_request_forge_url_into_url_field.py new file mode 100644 --- /dev/null +++ b/swh/web/add_forge_now/migrations/0008_turn_request_forge_url_into_url_field.py @@ -0,0 +1,18 @@ +# Generated by Django 2.2.28 on 2022-08-16 13:08 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("swh_web_add_forge_now", "0007_rename_denied_request_status"), + ] + + operations = [ + migrations.AlterField( + model_name="request", + name="forge_url", + field=models.URLField(), + ), + ] diff --git a/swh/web/add_forge_now/models.py b/swh/web/add_forge_now/models.py --- a/swh/web/add_forge_now/models.py +++ b/swh/web/add_forge_now/models.py @@ -105,7 +105,7 @@ submitter_forward_username = models.BooleanField(default=False) # FIXME: shall we do create a user model inside the webapp instead? forge_type = models.TextField() - forge_url = models.TextField() + forge_url = models.URLField() forge_contact_email = models.EmailField() forge_contact_name = models.TextField() forge_contact_comment = models.TextField( diff --git a/swh/web/templates/add_forge_now/creation_form.html b/swh/web/templates/add_forge_now/creation_form.html --- a/swh/web/templates/add_forge_now/creation_form.html +++ b/swh/web/templates/add_forge_now/creation_form.html @@ -45,7 +45,7 @@ Forge URL + name="forge_url" oninput="swh.add_forge.validateForgeUrl(this)" required> Remote URL of the forge. diff --git a/swh/web/tests/add_forge_now/test_migration.py b/swh/web/tests/add_forge_now/test_migration.py --- a/swh/web/tests/add_forge_now/test_migration.py +++ b/swh/web/tests/add_forge_now/test_migration.py @@ -17,6 +17,7 @@ MIGRATION_0005 = "0005_prepare_inbound_email" MIGRATION_0006 = "0006_request_add_new_fields" MIGRATION_0007 = "0007_rename_denied_request_status" +MIGRATION_0008 = "0008_turn_request_forge_url_into_url_field" def now() -> datetime: @@ -211,3 +212,41 @@ last_modified_date=datetime.now(timezone.utc), ) req.clean_fields() + + +def test_add_forge_now_url_validation(migrator): + + state = migrator.apply_tested_migration((APP_LABEL, MIGRATION_0007)) + Request = state.apps.get_model(APP_LABEL, "Request") + + from swh.web.add_forge_now.models import RequestStatus + + request = Request( + status=RequestStatus.PENDING.name, + submitter_name="dudess", + submitter_email="dudess@orga.org", + forge_type="cgit", + forge_url="foo", + forge_contact_email="forge@example.org", + forge_contact_name="forge", + forge_contact_comment="bar", + last_modified_date=datetime.now(timezone.utc), + ) + request.clean_fields() + + state = migrator.apply_tested_migration((APP_LABEL, MIGRATION_0008)) + Request = state.apps.get_model(APP_LABEL, "Request") + + request = Request( + status=RequestStatus.PENDING.name, + submitter_name="johndoe", + submitter_email="johndoe@example.org", + forge_type="cgit", + forge_url="foobar", + forge_contact_email="forge@example.org", + forge_contact_name="forge", + forge_contact_comment="bar", + last_modified_date=datetime.now(timezone.utc), + ) + with pytest.raises(ValidationError, match="Enter a valid URL."): + request.clean_fields() diff --git a/swh/web/tests/api/views/test_add_forge_now.py b/swh/web/tests/api/views/test_add_forge_now.py --- a/swh/web/tests/api/views/test_add_forge_now.py +++ b/swh/web/tests/api/views/test_add_forge_now.py @@ -3,6 +3,7 @@ # License: GNU Affero General Public License version 3, or any later version # See top-level LICENSE file for more information +import copy import datetime import threading import time @@ -199,6 +200,27 @@ assert len(requests) == 1 +@pytest.mark.django_db(transaction=True, reset_sequences=True) +def test_add_forge_request_create_invalid_forge_url(api_client, regular_user): + api_client.force_login(regular_user) + url = reverse("api-1-add-forge-request-create") + + forge_data = copy.deepcopy(ADD_FORGE_DATA_FORGE1) + forge_data["forge_url"] = "foo" + + resp = check_api_post_response( + api_client, + url, + data=forge_data, + status_code=400, + ) + + assert resp.data == { + "exception": "BadInputExc", + "reason": '{"forge_url": ["Enter a valid URL."]}', + } + + @pytest.mark.django_db(transaction=True, reset_sequences=True) def test_add_forge_request_update_anonymous_user(api_client): url = reverse("api-1-add-forge-request-update", url_args={"id": 1})