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

list of PRs to merge #7

Closed
DanielRuf opened this issue Jul 17, 2020 · 26 comments
Closed

list of PRs to merge #7

DanielRuf opened this issue Jul 17, 2020 · 26 comments

Comments

@DanielRuf
Copy link

DanielRuf commented Jul 17, 2020

Let's collect the open PRs that we have to merge. We already merged a few using the GitHub CLI.

For the needed steps see #2 (comment)

https://github.com/Dogfalo/materialize/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-desc

Already merged:

Fix some typos (found by codespell)
datepicker: add yearRangeReverse property sort year in reverse order
fix dropdown position when coverTrigger is false
Typos in upgrade guide

Planned to merge:

passive event receivers
Add input suffix
Add new i18n option 'headerFormat' to datepicker

@DanielRuf
Copy link
Author

@materializecss/members

@Smankusors
Copy link
Member

oh man, thanks for continuing the development of materialize 😄

Ehm I have this 2 years old pull request

Should I rewrite this pull request for this fork? 🤔

@DanielRuf
Copy link
Author

oh man, thanks for continuing the development of materialize 😄

Thanks, we all try our best to bring it a bit forward in our vailable free time.

Ehm I have this 2 years old pull request

I was not aware of this, good catch.

Should I rewrite this pull request for this fork? 🤔

Yes, this would be great and very helpful.

@Smankusors
Copy link
Member

Yes, this would be great and very helpful.

alright #9 it is 🙌

@DanielRuf
Copy link
Author

DanielRuf commented Sep 6, 2020

Dogfalo#5725 was mentioned in the community room.

@ChildishGiant
Copy link
Member

It's probably worth taking a look at Dogfalo#4391 (Dogfalo#1892) for the next feature release.

@doughballs
Copy link

Where are we up to regarding the select issue? That's a biggie - has it been raised/solved for the fork?

(The issue is the wrong item being selected on iOS, the fix being to served an entire patched JS file after materialize.js)

@Smankusors
Copy link
Member

Smankusors commented Sep 26, 2020

Where are we up to regarding the select issue? That's a biggie - has it been raised/solved for the fork?

(The issue is the wrong item being selected on iOS, the fix being to served an entire patched JS file after materialize.js)

hmm I think this is issue #31. There's already a commit Dogfalo@c0da340, but it's not released officially yet.

@DanielRuf
Copy link
Author

Correct, Dogfalo@c0da340 should be already in the current code.

@Pierstoval
Copy link

I haven't checked, but are the current CVEs resolved on the fork?

They are related to XSS flaws:

@DanielRuf
Copy link
Author

are the current CVEs resolved on the fork

Probably not, as fixing these would break many things as I have outlined many times (see the linked GitHub issues in the original repo). Nothing what we will do in a 1.x.x release.

Also the people who reported these findings did not provide valid PoCs that could be used by attackers in valid scenarios.

@Pierstoval
Copy link

It's a bit sad that these warnings will continue to spawn on projects that are using Materialize 😕

I just saw the discussion in Dogfalo#6331 and Dogfalo#6286, this explains a lot, but is still a complex subject.

Since this repository is "new", and the package will be new on NPM, maybe the vulnerability won't be registered at all, let's see 🤔

@Pierstoval
Copy link

Still, I think input data should be sanitized by default, with a simple option in the components, and user should be able to disable it in case they want to use HTML.

@DanielRuf
Copy link
Author

Still, I think input data should be sanitized by default, with a simple option in the components, and user should be able to disable it in case they want to use HTML.

But not in a minor or patch release as this will lead to unexpected consequences like broken code and developers need to be aware of this breaking change then.

We can do that in a 2.0.0 release which deprecates / removes and changes the behavior as you suggest.
But so far: developers should sanitize the data as needed. If you put your $_GET["var"] in the templates without sanitizing the data, you have a general problem regarding security.

In v1.x we can start adding warnings to the console (and hope that developers see/read them) and mention the deprecation/change in the future.

In general I would also suggest we create a documentation page about this and recommend solutions for sanitizing data with a few examples. How and what you sanitize depends on your implementation and software stack (PHP, JS, Java, ...) and where you retrieve and sanitize it.

@DanielRuf
Copy link
Author

Regarding this statement:

I noticed there's no patch version. As @kiere said, some clients don't really like the fact that we use a library with vulnerabilities, and even if I may not care (for a personal project for example), and I don't like enterprises' protocols, well, a project that's blocked because some QA says that "a package is vulnerable" is an issue anyway.

Well, jQuery < 3.x has a few vulnerabilities (like XSS) but you can not patch them without breaking stuff and these versions reached their EOL. That's why I've created https://github.com/DanielRuf/snyk-js-jquery-174006 and https://github.com/DanielRuf/snyk-js-jquery-565129 which are backported patches. And these are used by many like Drupal, WordPress, TYPO3, Magento and so on. Still, the Chrome Lighthouse checks and other solutions will falsly flag them as vulnerable versions. Same for PHP and the different operating systems and companies who backport security patches.

I'm aware that these things (backporting security patches) are not that great for auditors and audits.

Can you open a new issue so we can discuss the correct way (documentation, planned changes, breaking changes) regarding these issues @Pierstoval?

@Pierstoval
Copy link

Will do 😄

@Pierstoval
Copy link

Done in #38

@Sorc96
Copy link

Sorc96 commented Oct 20, 2020

It would be nice to see Dogfalo#6602 merged. It really should be possible to pass dropdownOptions to any component that uses a Dropdown.

@LeafHacker
Copy link

Have you seen PR Dogfalo#6542 / Issue Dogfalo#6273? Would be nice to see materialize keeping up with the MD guidelines

@DanielRuf
Copy link
Author

Have you seen PR Dogfalo#6542 / Issue Dogfalo#6273? Would be nice to see materialize keeping up with the MD guidelines

Nope, I didn't see these. Would be great to have them as PRs in this repo.
Do you want to provide them?

@LeafHacker
Copy link

Have you seen PR Dogfalo#6542 / Issue Dogfalo#6273? Would be nice to see materialize keeping up with the MD guidelines

Nope, I didn't see these. Would be great to have them as PRs in this repo.
Do you want to provide them?

I've just taken a closer look - and the PR doesn't actually make any changes to the text input scss - rather it's full of random/unrelated changes to the docs afaict. It does have a bunch of CSS in the description but it'd be a pain to diff this against master to see what it's actually changing... bizarre!

@DanielRuf
Copy link
Author

I've just taken a closer look - and the PR doesn't actually make any changes to the text input scss - rather it's full of random/unrelated changes to the docs afaict. It does have a bunch of CSS in the description but it'd be a pain to diff this against master to see what it's actually changing... bizarre!

You are correct. I just took a look at the issue and not the PR to see what this was about and thought it would make sense. I didn't check the actual changes of the PR.

@DanielRuf
Copy link
Author

Not sure if the comment with the scss code contains the actual changes and the other file changes were done by mistake (probably due to lack of experience with Git and GitHub).

@AJLeonardi
Copy link

Where are we up to regarding the select issue? That's a biggie - has it been raised/solved for the fork?
(The issue is the wrong item being selected on iOS, the fix being to served an entire patched JS file after materialize.js)

hmm I think this is issue #31. There's already a commit Dogfalo/materialize@c0da340, but it's not released officially yet.

Where are things with the DropDown issue on iOS (still present in current release)? There's a solution defined in #31, but it's unclear if it's been tested. Thanks!

@DanielRuf
Copy link
Author

Where are things with the DropDown issue on iOS (still present in current release)? There's a solution defined in #31, but it's unclear if it's been tested. Thanks!

Not sure. If there is still a problem with our alpha release, please open a new issue.

Closing this one. Individual PRs that should be migrated should be filed as separate issues, otherwise the issues become too long and crowded. Thank you.

@wuda-io
Copy link
Member

wuda-io commented Jan 15, 2022

I tested it on iPad and MacBook not sure about iPhone but I assume it would work there too. As @DanielRuf said, please open a new issue an provide details about Safari Version and iPhone. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants