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

Issue 1426 - Source validation on the claim form #1428

Open
wants to merge 9 commits into
base: stage
Choose a base branch
from

Conversation

lucaslobatob
Copy link
Collaborator

Description

BaseClaimForm.tsx: Creating source validation on the claim form

Fixes #1426

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Existing feature enhancement (non-breaking change which modifies existing functionality)

Testing

When placing an invalid source in the claim creation form, the form is not created and remains on the page until it has a valid source.
First test: Tested with one source valid, the user can send the form and create claim
Second test: Tested with one source valid and one source invalid, the user is unable to proceed with the creation of a claim, as it has an invalid source and the feedback: "A fonte não está mais disponível"
Third test: Tested with one source invalid, the user is unable to proceed with the creation of a claim, as it has an invalid source and the feedback: "A fonte não está mais disponível"
image

Developer Checklist

General

  • Code is appropriately commented, particularly in hard-to-understand areas
  • Repository documentation has been updated (Readme.md) with additional steps required for a local environment setup.
  • No console.log or related logging is added.
  • No code is repeated/duplicated in violation of DRY. The exception to this is for new (MVP/Prototype) functionality where the abstraction layer may not be clear (comments should be added to explain the violation of DRY in these scenarios).
  • Documented with TSDoc all library and controller new functions

Frontend Changes

  • No new styling is added through CSS files (Unless it's a bugfix/hotfix)
  • All types are added correctly

Tests

  • All existing unit and end to end tests pass across all services
  • Unit and end to end tests have been added to ensure backend APIs behave as expected

Merge Request Review Checklist

  • An issue is linked to this PR and these changes meet the requirements outlined in the linked issue(s)
  • High risk and core workflows have been tested and verified in a local environment.
  • Enhancements or opportunities to improve performance, stability, security or code readability have been noted and documented in JIRA issues if not being addressed.
  • Any dependent changes have been merged and published in downstream modules
  • Changes to multiple services can be deployed in parallel and independently. If not, changes should be broken out into separate merge requests and deployed in order.

…making a GET request.

- Backend: Created new route /api/source/check-source to handle URL validation and return link status.
- Frontend: Updated ClaimSourceListItem component to call API for link verification and handle valid/invalid links.
…ion, handle valid/invalid links and created message feedback for user
@thesocialdev thesocialdev force-pushed the stage branch 6 times, most recently from 05f8912 to 864c35e Compare November 7, 2024 18:27
@thesocialdev
Copy link
Collaborator

@lucaslobatob cypress tests are failing after this changes

@@ -103,6 +103,23 @@ export class SourceController {
);
}

@ApiTags("source")
@Get("/check-source")
Copy link
Collaborator

Choose a reason for hiding this comment

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

issue(blocking): now that I'm thinking, this is a DDOS vector and we should not continue. We can consider a few options like putting this behind RBAC for logged-in users.

But why aren't we just requesting from the client-side? Why we need to create a new endpoint for this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The endpoint was needed because of CORS restrictions, many sources block client-side requests. After, I can look with @lucaslobatob at some ways to make it more secure.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm still not satisfied with the current solution, there are still too many vulnerabilities. Let's sync and create an epic documentation before proceeding.

@@ -103,6 +103,23 @@ export class SourceController {
);
}

@ApiTags("source")
@Get("/check-source")
async checkSpurce(@Query() query) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick: typo

@thesocialdev thesocialdev mentioned this pull request Nov 12, 2024
17 tasks
server/source/source.service.ts Dismissed Show dismissed Hide dismissed
… API to check the validity of a URL before opening it
@@ -142,7 +142,7 @@ export class ClaimController {
: path,
};
} catch (error) {
return error;
Copy link
Collaborator

Choose a reason for hiding this comment

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

issue: if you're doing this the try/catch is not needed.

@ApiTags("source")
@Get("/check-source")
@UseGuards(AbilitiesGuard)
@CheckAbilities(new RegularUserAbility())
Copy link
Collaborator

Choose a reason for hiding this comment

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

issue: this should be a FactChecker, regular users can't create resource anyway.

@@ -103,6 +103,23 @@ export class SourceController {
);
}

@ApiTags("source")
@Get("/check-source")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm still not satisfied with the current solution, there are still too many vulnerabilities. Let's sync and create an epic documentation before proceeding.

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.

Prevent invalid source creation during claim workflow
3 participants