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

Fixes for #2891 #2889 #2897

Merged
merged 7 commits into from
Apr 10, 2024
Merged

Fixes for #2891 #2889 #2897

merged 7 commits into from
Apr 10, 2024

Conversation

atarix83
Copy link
Contributor

@atarix83 atarix83 commented Apr 5, 2024

References

Description

This is a small follow-up PR to fix the mentioned issues

Checklist

  • My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
  • My PR passes ESLint validation using yarn lint
  • My PR doesn't introduce circular dependencies (verified via yarn check-circ-deps)
  • My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
  • My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
  • If my PR includes new libraries/dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
  • If my PR includes new features or configurations, I've provided basic technical documentation in the PR itself.
  • If my PR fixes an issue ticket, I've linked them together.

@tdonohue tdonohue added bug 1 APPROVAL pull request only requires a single approval to merge labels Apr 5, 2024
@atarix83
Copy link
Contributor Author

atarix83 commented Apr 5, 2024

@tdonohue @artlowel

regarding the CodeQL alert, i've added some checks how is described in the documentation but it persists. Do you have any suggestion, could we ignore it or do i need to investigate further?

@artlowel
Copy link
Member

artlowel commented Apr 8, 2024

@atarix83 The suggestion from CodeQL is to turn LazyDataServicesMap into an actual Map object:

in data-services.map.ts:

export const LAZY_DATA_SERVICES = new Map<string, () => Promise<Type<HALDataService<any>>>>()
LAZY_DATA_SERVICES.set(AUTHORIZATION.value, () => import('./data/feature-authorization/authorization-data.service').then(m => m.AuthorizationDataService));
...

in lazy-service pass both the map and the key, so you can write something like:

if (map.has(key)) {
   const loader = map.get(key):
   loader();
} else {
   trow new Error(`The key ${key} was not found in the map ${map}`);
}

where map has the type Map<string, () => Promise<any>> in the method to ensure that loader() is indeed a function that returns a promise. If that doesn't appease CodeQL, you may need to explicitly test that loader is a function as well.

@atarix83
Copy link
Contributor Author

atarix83 commented Apr 9, 2024

thanks @artlowel for suggestions, now every checks pass and PR is to be reviewed

Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Thanks @atarix83 ! I've reviewed the code and it looks reasonable. CodeQL also now approves it. I've also tested to make sure that no obvious bugs exist... I could not find any issues & I'm able to verify that the CSRF token is still working as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 APPROVAL pull request only requires a single approval to merge bug
Projects
No open projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Refactor initial CSRF creation into an Effect Resolve CodeQL alert for "Unvalidated dynamic method call"
3 participants