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

We need a smarter occ app:check-code #17187

Closed
oparoz opened this issue Jun 25, 2015 · 19 comments
Closed

We need a smarter occ app:check-code #17187

oparoz opened this issue Jun 25, 2015 · 19 comments

Comments

@oparoz
Copy link
Contributor

oparoz commented Jun 25, 2015

Devs have no choice but use some private APIs which have no OCP equivalent and by doing so, their apps are being flagged as being "not compliant". Those methods should be listed, but have no impact on the app's compliance status.

Also, the app checker should treat code located in the vendor folders differently.
Issues should still be reported, so that an admin can estimate the quality of the libs chosen to support the app, but it shouldn't have an impact on the app's compliance status.

@RobinMcCorkell
Copy link
Member

We currently ignore certain classes that have no corresponding public API, but clearly there are some that we falsely include. Could you list some of them? In addition, we should be creating public APIs when app developers flag up app compliance issues like this. @rullzer has been doing a lot of work in this regard.

@oparoz
Copy link
Contributor Author

oparoz commented Jun 25, 2015

Ah, I didn't know that. I didn't bother checking the code 😳

Here are some

\OC_User::setIncognitoMode
\OC_Util::tearDownFS()
\OC_Util::setupFS()

@karlitschek
Copy link
Contributor

The right way to fix it is to add this OCP calls that are needed. No dirty workarounds please :-)

@oparoz
Copy link
Contributor Author

oparoz commented Jun 25, 2015

Of course this needs fixing via OCP (it's taking years), but you can't have a compliance test which contains sections which nobody can pass.
As long as the whitelist is maintained, then you have an accurate representation of the landscape.
There is not a single official app which passes. 2 come close (activity and files_texteditor), but they don't have a public facing side.

@karlitschek
Copy link
Contributor

Not sure why it has to take years. Just request a missing call and it can be added to the next version. Most likely also back ported to the next patch release if the OCP is only a wrapper without logic.
Should be a matter of 3-5 weeks.

@oparoz
Copy link
Contributor Author

oparoz commented Jun 25, 2015

The original request to fix OC/OCP is from 2013 #4863 and while good progress has been made, especially these past months, the methods I've mentioned are not that easy to bring to the public space, especially setIncognitoMode #12888

The encryption app was holding things up. Maybe things will be easier with 2.0, but there needs to be a move from using View to using Node and to move the whole sharing architecture to the public space. Very few people can do that, so I don't think we'll be able to use these methods before next year and that means no compliance until then.

@oparoz oparoz added the dev:ocp label Jun 25, 2015
@DeepDiver1975
Copy link
Member

Also, the app checker should treat code located in the vendor folders differently.

these folders are not scanned

@DeepDiver1975 DeepDiver1975 added this to the 8.2-next milestone Jun 26, 2015
@DeepDiver1975
Copy link
Member

The original request to fix OC/OCP is from 2013 #4863 and while good progress has been made,

I think #4863 should be closed and every single time anybody encounters some missing api: please open a new issue - THX

@oparoz oparoz changed the title We need a smarter occ app:checker We need a smarter occ app:check-code Jun 26, 2015
@oparoz
Copy link
Contributor Author

oparoz commented Jun 26, 2015

Also, the app checker should treat code located in the vendor folders differently.

these folders are not scanned

Not true

Analysing /apps/galleryplus/js/vendor/bigshot/bigshot.php
 4 errors
    line   55: != - is discouraged
    line   55: != - is discouraged
    line   64: != - is discouraged
    line   76: == - is discouraged

Should I fill a bug report then?

I think #4863 should be closed and every single time anybody encounters some missing api: please open a new issue - THX

OK!

@DeepDiver1975
Copy link
Member

Analysing /apps/galleryplus/js/vendor/bigshot/bigshot.php

seriously - why is there php code in the js folder?

@oparoz
Copy link
Contributor Author

oparoz commented Jun 26, 2015

Devs don't control what the vendor provides in the package they use and usually all files have to be distributed.
As stated in the OP, I think it's still useful to scan those folders, but only to detect malicious code, not bad practices.

@LukasReschke
Copy link
Member

Then it simply is a crappy library. One shall not ship not required files.

I mean: Seriously a Java Applet? (Same-Origin-Policy anyone?) And a bigshot.php script that is insecure if your PHP version is vulnerable to Null Byte attacks? (a ton are – RHEL only fixed it for example because we complained loud enough) And also insecure if your script runs on Windows?

There is a reason why our libs that we ship all have some exclusions using the .gitignore file.

@oparoz
Copy link
Contributor Author

oparoz commented Jun 26, 2015

I'll just distribute the archive along with the JS then.

@oparoz
Copy link
Contributor Author

oparoz commented Jun 26, 2015

\OC_Util::setupFS() #17196
\OC_Util::tearDownFS() #17197
\OC_User::setIncognitoMode #12888

@DeepDiver1975
Copy link
Member

\OC_Util::setupFS() #17196
\OC_Util::tearDownFS() #17197
\OC_User::setIncognitoMode #12888

thx - we will come up a proper public api for this in 8.2

@oparoz
Copy link
Contributor Author

oparoz commented Jun 26, 2015

Great news 🎩

@BernhardPosselt
Copy link
Contributor

👎 I don't see the use cases for these things, you can just get the user folder and everything works fine ™️

@oparoz
Copy link
Contributor Author

oparoz commented Jul 2, 2015

It doesn't when you only have a token to work with... and that's the main problem. Apps can't serve the content of public shares using only public APIs.

@DeepDiver1975 DeepDiver1975 modified the milestones: 9.0-next, 8.2-current Sep 21, 2015
@ghost ghost modified the milestones: 9.1-next, 9.0-current Feb 22, 2016
@ghost ghost added the old-inactive label Feb 22, 2016
@PVince81 PVince81 modified the milestones: 9.1-current, 9.2-next Jun 14, 2016
@PVince81 PVince81 removed this from the 9.1-current milestone Jun 14, 2016
@PVince81 PVince81 modified the milestones: backlog, 10.0 Dec 8, 2016
@stale
Copy link

stale bot commented Sep 20, 2021

This issue has been automatically closed.

@stale stale bot closed this as completed Sep 20, 2021
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