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

Ajaxify Cors section in Personal settings #37560

Merged
merged 1 commit into from
Jun 22, 2020
Merged

Ajaxify Cors section in Personal settings #37560

merged 1 commit into from
Jun 22, 2020

Conversation

VicDeo
Copy link
Member

@VicDeo VicDeo commented Jun 19, 2020

Description

CORS section silently fail when a new domain is incorrect

Motivation and Context

How Has This Been Tested?

  1. Open Settings -> Personal -> Security
  2. Add domain e.g. 'localhost' to the CORS section

Expected

Domain is not added, Error message

Actual

Domain is not added, No error message

Screenshot

Screenshot_20200622_165612

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

@update-docs
Copy link

update-docs bot commented Jun 19, 2020

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@VicDeo VicDeo self-assigned this Jun 19, 2020
@VicDeo VicDeo added this to the development milestone Jun 19, 2020
@VicDeo VicDeo force-pushed the bugfix/e4050 branch 4 times, most recently from 0be3ced to b4ee363 Compare June 19, 2020 14:54
@VicDeo VicDeo changed the title Ajaxify Cors secion in Personal settings Ajaxify Cors section in Personal settings Jun 19, 2020
@VicDeo VicDeo force-pushed the bugfix/e4050 branch 3 times, most recently from 0b8d577 to 03eab90 Compare June 19, 2020 15:32
@codecov
Copy link

codecov bot commented Jun 19, 2020

Codecov Report

Merging #37560 into master will increase coverage by 0.03%.
The diff coverage is 92.85%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #37560      +/-   ##
============================================
+ Coverage     64.66%   64.69%   +0.03%     
- Complexity    19343    19345       +2     
============================================
  Files          1279     1279              
  Lines         75600    75589      -11     
  Branches       1333     1333              
============================================
+ Hits          48885    48902      +17     
+ Misses        26323    26295      -28     
  Partials        392      392              
Flag Coverage Δ Complexity Δ
#javascript 54.03% <ø> (ø) 0.00 <ø> (ø)
#phpunit 65.87% <92.85%> (+0.03%) 19345.00 <4.00> (+2.00)
Impacted Files Coverage Δ Complexity Δ
settings/Application.php 53.65% <0.00%> (-0.33%) 2.00 <0.00> (ø)
settings/routes.php 100.00% <ø> (ø) 0.00 <0.00> (ø)
settings/Controller/CorsController.php 97.56% <100.00%> (+0.06%) 10.00 <4.00> (ø)
settings/templates/panels/personal/cors.php 100.00% <100.00%> (+100.00%) 0.00 <0.00> (ø)
lib/private/Archive/Archive.php 65.00% <0.00%> (-3.43%) 11.00% <0.00%> (+1.00%) ⬇️
..._sharing/lib/Controller/NotificationController.php 42.10% <0.00%> (ø) 14.00% <0.00%> (ø%)
lib/private/Share/MailNotifications.php 95.74% <0.00%> (+0.09%) 33.00% <0.00%> (ø%)
settings/Panels/Personal/Cors.php 100.00% <0.00%> (+50.00%) 4.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab72fd5...4c96790. Read the comment docs.

@VicDeo VicDeo force-pushed the bugfix/e4050 branch 2 times, most recently from b54a7c4 to 3bcc4bd Compare June 19, 2020 17:19
@VicDeo VicDeo requested review from pmaier1, micbar and jvillafanez June 19, 2020 19:33
@micbar
Copy link
Contributor

micbar commented Jun 22, 2020

@VicDeo can you point to the Support case?

@VicDeo
Copy link
Member Author

VicDeo commented Jun 22, 2020

\array_push($domains, $domain);

// In case same domain is added
$domains = \array_unique($domains);
Copy link
Member

Choose a reason for hiding this comment

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

If the initial domain list is already sanitized, an in_array(...) check should be faster to know if the domain is there.
If we don't trust the list is sanitized, we should assume the same in every place where we're retrieving the list from the user's preferences (removeDomain function would just remove one domain, but not the duplicates, if any)

Copy link
Member Author

@VicDeo VicDeo Jun 22, 2020

Choose a reason for hiding this comment

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

I didn't touch this code - just wrapped it with if condition https://github.com/owncloud/core/pull/37560/files?w=1

In addition it is not something that expected to have millions of domains and to be used on daily basis.

@@ -1,33 +1,67 @@
var PersonalCors = {
renderRow: function(domain){
var row = $('<tr />').appendTo('#cors .grid tbody');
Copy link
Member

Choose a reason for hiding this comment

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

Maybe something like var htmlRow = '<tr><td>' + domain + '</td><td><input ..... /></td></tr>' (with a readable format) is easier to follow?
Then just $('#cors .grid tbody').append(htmlRow);

Copy link
Member Author

Choose a reason for hiding this comment

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

nope, what if domain contains <script>alert(1)</script>

Copy link
Member

Choose a reason for hiding this comment

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

Well, I'd expect the domain to be sanitized at that point... 😅
Maybe...

var row = $('<tr></tr>')
    .append($('<td></td>').text(domain))
    .append('<td><input ..... /></td>')
$('#cors .grid tbody').append(row);

or maybe

var col1 = $('<td></td>').text(domain);
var col2 = '<td><input .... /></td>';
var row = $('<tr></tr>').append(col1).append(col2);
$('#cors .grid tbody').append(row);

anyway, up to you.

settings/js/panels/cors.js Outdated Show resolved Hide resolved
settings/js/panels/cors.js Outdated Show resolved Hide resolved
Copy link
Member

@jvillafanez jvillafanez left a comment

Choose a reason for hiding this comment

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

There is only one requested change for readability, but not critical. Up to @VicDeo to decide

@VicDeo VicDeo merged commit a00203d into master Jun 22, 2020
@delete-merged-branch delete-merged-branch bot deleted the bugfix/e4050 branch June 22, 2020 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants