Sam Ruby wrote on 6/9/17 11:46 AM: > On Fri, Jun 9, 2017 at 11:02 AM, Shane Curcuru <a...@shanecurcuru.org> wrote: >> In building the public-orgchart branch, I'd like to ask: >> >> * whimsy.a.o/orgchart or whimsy.a.o/docs/orgchart or >> whimsy.a.o/foundation/orgchart ? > > I prefer more specific (foundation) over generic (docs).
ACK, thx. > >> * For a code review of some parts before we merge to master: >> >> https://github.com/apache/whimsy/blob/public-orgchart/www/docs/orgchart.cgi#L66 >> Since this cgi will publicly expose bits of formerly private data in >> foundation/officers/personnel-duties, can someone double-check that >> fields marked 'private' in the .yamls won't leak? > > I've gone through each page, and it looks right to me. > >> https://github.com/apache/whimsy/blob/public-orgchart/lib/whimsy/asf/orgchart.rb#L13 >> Since this is a private repo, is there anything else we explicitly need >> to check before .untaint in this class? > > ASF::SVN should already untaint this value. If it doesn't, it is a > bug in ASF::SVN. I got some insecure errors testing locally that went away with .untaint, but then again, my local environment is a bit mucked up, so perhaps my adding that was just unneeded voodoo. > >> https://github.com/apache/whimsy/blob/public-orgchart/www/docs/orgchart.cgi#L129 >> Why doesn't my use of _markdown work, when it seems identical to the one >> in www/roster/views/duties.html.rb? > > I also pushed a small change, enabling markdown. Ah-ha. That makes more sense; I assumed it was included already. > >> * Consider changing some of the personnel-duties data structure: >> >> - The [info] id: field should really be a list, not a string - this >> allows the board and infra-staff groups to properly reflect they are >> groups of committers, not single individuals. Any objections? > > That would make this page harder to read: http://whimsy.local/docs/orgchart > > Perhaps an new field? Perhaps id: is a list, and chair: is a single value; keeping the front page simple, but enabling better output on the per-role pages? > >> * TODO: deprecate www/roster/models/orgchart.rb in favor of >> lib/whimsy/asf/orgchart.rb > > What do you propose for private fields? Um, that they would be handled by any display code? - If there's significant risk that someone can write untrusted display code against this model, then it needs to be changed to explicitly drop all private fields inside the model. - But since only committers can push code, I'm not sure if that's a realistic danger. If we don't trust whimsy committers, then we're already in trouble if a cgi in an un-auth'd directory can access most of the ASF:: classes and get data out of them. Anyway, for the time being, I'll leave them separate. -- - Shane https://www.apache.org/foundation/marks/resources
