Skip to content

Critical code review PR - Last sprint handover checklist of 0.15.0 release#544

Merged
Prafulrakhade merged 0 commit intomasterfrom
release-0.15.x
Jan 29, 2025
Merged

Critical code review PR - Last sprint handover checklist of 0.15.0 release#544
Prafulrakhade merged 0 commit intomasterfrom
release-0.15.x

Conversation

@vishwa-vyom
Copy link
Contributor

This PR is created just to add the review comments as part of the critical code review task of last sprint handover checklist for release of 0.15.0 version.

** THIS PR SHOULD NOT BE MERGED **

log.error("Wallet binding error occured for tranaction id " + requestDTO.getRequest().getIndividualId(), e);
String[] errorObj = Utilities.handleExceptionWithErrorCode(e);
List<ErrorDTO> errors = Utilities.getErrors(errorObj[0], errorObj[1]);
responseWrapper.setResponse(null);

Check failure

Code scanning / CodeQL

Server-side request forgery

Potential server-side request forgery due to a [user-provided value](1).
Copy link
Contributor Author

@vishwa-vyom vishwa-vyom left a comment

Choose a reason for hiding this comment

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

Reviewed file changes and added comments excluding the test and api-test files.

- ./config/credential-template.html:/config/server/credential-template.html
- ./nginx.conf:/etc/nginx/nginx.conf
mimoto-service:
container_name: 'Mimoto-Service'
image: 'mosipid/mimoto:0.12.0'
image: 'mosipid/mimoto:0.14.0'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is this not suppose to be mosipdev with release-0.15.0 tag ?

Copy link
Contributor

Choose a reason for hiding this comment

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

mosipqa/release-0.15.x

Copy link
Contributor Author

@vishwa-vyom vishwa-vyom Jan 16, 2025

Choose a reason for hiding this comment

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

I see this is not yet changed, not sure if QA verified with correct version

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

updated now #557

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, but QA needs to test this docker compose also right ?

}
}
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This level of validation is required ? Do we have a story/task on this ?

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to support two formats ldp and mso_mDoc where responses are different. https://mosip.atlassian.net/browse/INJIMOB-1862

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this part of release scope ?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes @vishwa-vyom
it's part of release and mobile QAs have tested mDL VCs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed this is getting captured directly in the release notes

@Prafulrakhade Prafulrakhade merged commit 78190fd into master Jan 29, 2025
27 of 30 checks passed
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.

4 participants