Updated check-ueid.js to handle html elements #4416
Conversation
safely to prevent xss
|
Terraform plan for meta No changes. Your infrastructure matches the configuration.📝 Plan generated in Pull Request Checks #3863 |
|
Terraform plan for dev Plan: 1 to add, 0 to change, 1 to destroy.Terraform used the selected providers to generate the following execution
plan. Resource actions are indicated with the following symbols:
-/+ destroy and then create replacement
Terraform will perform the following actions:
# module.dev.module.cors.null_resource.cors_header must be replaced
-/+ resource "null_resource" "cors_header" {
!~ id = "*******************" -> (known after apply)
!~ triggers = { # forces replacement
!~ "always_run" = "2024-10-22T12:25:56Z" -> (known after apply)
}
}
Plan: 1 to add, 0 to change, 1 to destroy.
Warning: Argument is deprecated
with module.dev-backups-bucket.cloudfoundry_service_instance.bucket,
on /tmp/terraform-data-dir/modules/dev-backups-bucket/s3/main.tf line 14, in resource "cloudfoundry_service_instance" "bucket":
14: recursive_delete = var.recursive_delete
Since CF API v3, recursive delete is always done on the cloudcontroller side.
This will be removed in future releases
(and 6 more similar warnings elsewhere)📝 Plan generated in Pull Request Checks #3863 |
Minimum allowed line rate is |
|
These changes look fine to me, but it's important to call out that they appear to be copied and pasted from the GitHub Copilot suggestion (with some reorganizing). I trust that the reviewers knew this and discussed it, but we need to be very careful about incorporating AI-derived code into the codebase. These changes are relatively minor, but it's still worth reminding everyone. |
That is correct @danswick, it is good to review AI generated suggestions, which I did. This is very similar to the co-pilot suggestion because there was no reason to change it very much other than some organization as its suggested implementation accomplished the goal, which was correcting the potential for XSS by placing in tags/strings into into the object model instead of a string with tags. The logic was not complex. |
|
I'll be out of the room for a while, so the team will want to discuss some norms around the AI code.
(For reference, when you're reading SO, their license is pretty explicit, and generally compatible as long as we reference our work (BY-SA). https://stackoverflow.com/help/licensing) In this regard... my personal inclination would be to never use it. However, the team can discuss and perhaps do an ADR about it. |
asteel-gsa
dismissed stale reviews from jperson1 and phildominguez-gsa
Dismissing pending discussion
|
Dismissing reviews at this time for re-review. Proposed: const dl = document.createElement('dl');
const dtUei = document.createElement('dt');
const ddUei = document.createElement('dd');
const dtName = document.createElement('dt');
const ddName = document.createElement('dd');
dl.setAttribute('data-testid', 'uei-info');
dtUei.textContent = 'Unique Entity ID';
ddUei.textContent = auditeeUei;
dtName.textContent = 'Auditee name';
ddName.textContent = auditeeName.value;
dl.appendChild(dtUei);
dl.appendChild(ddUei);
dl.appendChild(dtName);
dl.appendChild(ddName);
ueiInfoEl.appendChild(dl);Appears to be reorganized, utilizing the same html structure we have, however, I agree with Matt and Dan in this regard. We will likely need to review our license https://creativecommons.org/publicdomain/zero/1.0/legalcode on top of finding out the status at a TTS level. Last I have seen through slack is usage is not permitted, at least in relation to plugin via VSCode. We would need a definitive "CoPilot usage is okay" from TTS at large, and I do not think that has been given. |
|
@anagradova spoke with @jperson1 and this would be my proposed change to get it through. CC: @danswick Refs:
let dl; let dtUei; let ddUei; let dtName; let ddName;
dl = document.createElement('dl');
dtUei = document.createElement('dt');
ddUei = document.createElement('dd');
dtName = document.createElement('dt');
ddName = document.createElement('dd');
dl.setAttribute('data-testid', 'uei-info');
dtUei.textContent = 'Unique Entity ID';
ddUei.textContent = auditeeUei;
dtName.textContent = 'Auditee name';
ddName.textContent = auditeeName.value;
dl.append(dtUei,ddUei,dtName,ddName);
ueiInfoEl.appendChild(dl); |
safely to prevent xss
PR Checklist: Submitter
Issue: #4411
Steps to validate:
PR Checklist: Reviewer
make docker-clean; make docker-first-run && docker compose up; then rundocker compose exec web /bin/bash -c "python manage.py test"The larger the PR, the stricter we should be about these points.
Pre Merge Checklist: Merger
-/+ resource "null_resource" "cors_header"should be destroying and recreating its self and~ resource "cloudfoundry_app" "clamav_api"might be updating itssha256for thefac-file-scannerandfac-av-${ENV}by default.main.