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.

Sign up for GitHub

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

Jump to bottom

DOI / Update DOI widget in editor #8468

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

DOI / Update DOI widget in editor #8468

wants to merge 2 commits into 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

@fxprunayre
DOI / Update DOI widget in editor
5668327 
* 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
github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 25, 2024
View reviewed changes
web-ui/src/main/resources/catalog/components/doi/DoiService.js
@@ -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/
Loading
' can be anywhere in the URL, and arbitrary hosts may come before or after it.

Copilot Autofix AI 8 days 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
web-ui/src/main/resources/catalog/components/doi/DoiService.js
!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/
Loading
' can be anywhere in the URL, and arbitrary hosts may come before or after it.
@sonarcloud SonarCloud
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@josegar74 josegar74 Awaiting requested review from josegar74

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.4.7
Development

Successfully merging this pull request may close these issues.
1 participant
@fxprunayre