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.

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

DOI / Update DOI widget in editor #8468

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

fxprunayre
Copy link
Member

  • DOI server can be retrieved by editor by id or uuid
  • Updating a DOI is only allowed when a server is configured for the DOI prefix
  • DOI server list is disabled in update mode

image

Follow up of #8098

Checklist

  • I have read the contribution guidelines
  • Pull request provided for main branch, backports managed with label
  • Good housekeeping of code, cleaning up comments, tests, and documentation
  • Clean commit history broken into understandable chucks, avoiding big commits with hundreds of files, cautious of reformatting and whitespace changes
  • Clean commit messages, longer verbose messages are encouraged
  • API Changes are identified in commit messages
  • Testing provided for features or enhancements using automatic tests
  • User documentation provided for new features or enhancements in manual
  • Build documentation provided for development instructions in README.md files
  • Library management using pom.xml dependency management. Update build documentation with intended library use and library tutorials or documentation

Funded by Ifremer

* DOI server can be retrieved by editor by id or uuid
* Updating a DOI is only allowed when a server is configured for the DOI prefix
* DOI server list is disabled in update mode
@fxprunayre fxprunayre added this to the 4.4.7 milestone Oct 25, 2024
@@ -85,24 +108,55 @@
* the metadata is published.
*
*/
function canPublishDoiForResource(md, resource) {
function canPublishDoiForResource(md, doiUrl) {
if (doiUrl == null || doiUrl.indexOf("doi.org/") === -1) {

Check failure

Code scanning / CodeQL

Incomplete URL substring sanitization High

'
doi.org/
' can be anywhere in the URL, and arbitrary hosts may come before or after it.

Copilot Autofix AI about 1 month ago

To fix the problem, we need to ensure that the URL is parsed and the host is explicitly checked against a whitelist of allowed hosts. This will prevent attackers from embedding "doi.org/" in unexpected parts of the URL. We will use the URL constructor available in modern JavaScript to parse the URL and then check the host.

  1. Parse the doiUrl using the URL constructor.
  2. Extract the host from the parsed URL.
  3. Check if the host matches the expected "doi.org" or any allowed subdomains.
Suggested changeset 1
web-ui/src/main/resources/catalog/components/doi/DoiService.js

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/web-ui/src/main/resources/catalog/components/doi/DoiService.js b/web-ui/src/main/resources/catalog/components/doi/DoiService.js
--- a/web-ui/src/main/resources/catalog/components/doi/DoiService.js
+++ b/web-ui/src/main/resources/catalog/components/doi/DoiService.js
@@ -111,3 +111,12 @@
       function canPublishDoiForResource(md, doiUrl) {
-        if (doiUrl == null || doiUrl.indexOf("doi.org/") === -1) {
+        if (doiUrl == null) {
+          return false;
+        }
+        try {
+          var parsedUrl = new URL(doiUrl);
+          var host = parsedUrl.host;
+          if (host !== "doi.org" && !host.endsWith(".doi.org")) {
+            return false;
+          }
+        } catch (e) {
           return false;
EOF
@@ -111,3 +111,12 @@
function canPublishDoiForResource(md, doiUrl) {
if (doiUrl == null || doiUrl.indexOf("doi.org/") === -1) {
if (doiUrl == null) {
return false;
}
try {
var parsedUrl = new URL(doiUrl);
var host = parsedUrl.host;
if (host !== "doi.org" && !host.endsWith(".doi.org")) {
return false;
}
} catch (e) {
return false;
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
!isMdWorkflowEnableForMetadata
);
}

function checkDoiManagementForResource(md, resource) {
if (resource.locUrl == null || resource.locUrl.indexOf("doi.org/") === -1) {

Check failure

Code scanning / CodeQL

Incomplete URL substring sanitization High

'
doi.org/
' can be anywhere in the URL, and arbitrary hosts may come before or after it.
Copy link

sonarcloud bot commented Nov 12, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@fxprunayre fxprunayre requested a review from josegar74 November 12, 2024 16:19
@CLAassistant
Copy link

CLAassistant commented Dec 8, 2024

CLA assistant check
All committers have signed the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants